Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce an order for types when fishing #19

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

mint-thompson
Copy link
Collaborator

Completes task CIMPL-1184.

FISHING_ORDER defines the order in which definitions will be checked when fishing. If a different order is desired, fish for types one at a time.

Given that there was an existing test that specifically checked for the opposite behavior, this probably qualifies as a breaking change. We should keep that in mind when releasing this feature.

FISHING_ORDER defines the order in which definitions will be checked
when fishing. If a different order is desired, fish for types one at a
time.
Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh, interesting that we already had a test that did the opposite. But I guess that makes sense that it was there!

I think technically because FPL is not at a 1.x.y version yet, breaking changes can be introduced in minor version bumps pre-1.0. That said, SUSHI has been using FPL in an official release for a while now, so it probably is worth considering releasing an official 1.x version. Maybe now is our time!

Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the implementation approach. This looks good. I also ran this through regression and it did not affect any projects that were updated in the last year.

@mint-thompson mint-thompson merged commit fe1056d into main Oct 16, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants