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

Change docs and tests to use new style parameterized types #490

Merged
merged 4 commits into from
Aug 19, 2024

Conversation

martosaur
Copy link
Contributor

I was wrong, fields defined as {:parameterized, Ecto.Enum, Ecto.Enum.init(values: ...)} are passed as-is and thus don't work with new Ecto. I added a missing test to cover this case.

Flop needs to either manually translate them to preserve backwards compatibility or add this to upgrading guide 😢 This PR assumes the latter, but don't feel pushed to proceed with this.

@martosaur martosaur requested a review from woylie as a code owner August 18, 2024 18:10
Copy link

codecov bot commented Aug 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.67%. Comparing base (77dbd70) to head (d2e2a58).
Report is 17 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #490      +/-   ##
==========================================
+ Coverage   87.43%   87.67%   +0.24%     
==========================================
  Files          15       15              
  Lines         931      933       +2     
==========================================
+ Hits          814      818       +4     
+ Misses        117      115       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@martosaur
Copy link
Contributor Author

Re: coverage. I'm not quite sure how to realistically trigger expand_type function. It seems like in most cases it's preceded by module.get_field_info call which translates definition returned from an adapter into an Ecto type. The only case I can think of is if someone called allowed_operators/1 for a field definition they got somewhere else, which I'm not sure makes a lot of sense?

@woylie
Copy link
Owner

woylie commented Aug 18, 2024

Re: coverage. I'm not quite sure how to realistically trigger expand_type function. It seems like in most cases it's preceded by module.get_field_info call which translates definition returned from an adapter into an Ecto type. The only case I can think of is if someone called allowed_operators/1 for a field definition they got somewhere else, which I'm not sure makes a lot of sense?

Right, Flop.Schema expands the type at compile time already. I don't know whether there's a use case for this, but Flop.FieldInfo.ecto_type is a Flop.Schema.ecto_type(), which includes :from_schema and :ecto_enum, and allowed_operators accepts a FieldInfo struct or a Flop.Schema.ecto_type value directly. So let's just add tests for each combination to the allowed_operators tests for completeness.

@woylie woylie force-pushed the am/parameterized_type branch from 32e0658 to d2e2a58 Compare August 19, 2024 00:53
@woylie woylie merged commit 05c0ffc into woylie:main Aug 19, 2024
17 checks passed
@martosaur martosaur deleted the am/parameterized_type branch August 19, 2024 01:02
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.

2 participants