feat: support polymorphic associations / abstract schemas #51
Conversation
lib/ecto_shorts/common_filters.ex
Outdated
| ) :: Ecto.Query.t | ||
| def convert_params_to_filter(query, params) when params === %{} do | ||
| query | ||
| query_or_queryable_or_source_queryable :: query() | queryable() | source_queryable(), |
There was a problem hiding this comment.
I would just simplify this to query. It makes it more readable, and you're using it as a query within the context of the function. In it's current form the name is to hyper specific and a little redundant considering you have the definition to the right of the name that tells you exactly what types you expect.
There was a problem hiding this comment.
Thanks, I'm still working on a review, there is a lot to digest here.
There was a problem hiding this comment.
No worries, thanks for taking the time to look at it @BobbieBarker! For the areas that are unclear feel free to raise them and I can provide more information. Ecto is pretty huge in terms of functionality so I think there is opportunities to improve on the documentation as well for how it's supported.
There was a problem hiding this comment.
Some of this could also be split out into smaller prs as well taking a second look at this. For eg the refactor of the less public api such as the QueryBuilder.Common / QueryBuilder.Schema could be it's own pr.
There was a problem hiding this comment.
Did you use git mv when you moved these files around? I suspect you didn't, and if you had it would help it not look like you rewrote the library with this pr. 😆.
Also It seems to me that the chief value proposition in this PR is support for polymorphic associations through EctoShorts as described in the docs.
Is that true?
There was a problem hiding this comment.
-
No i did not use
git mv, good catch. I could go back to the last commit on main and then re-play those changes withgit mvand push it back up. Is that okay? -
Yes the main goal is to support polymorphic associations as described in the documentation. When developing the feature I didn't want to repeat the code and the handling for the syntax so
EctoShorts.CommonSchemaswas created as interface to handle the translation based on whether{source, schema}orschemais given.
There was a problem hiding this comment.
- I think that would go a long ways to making the PR easier to review for the changes you're introducing. Thank you.
- I think we should start considering what sorts of documentation we could introduce to help make users aware of this functionality, and make sure they understand how to take advantage of it etc. -- If this already exists and I didn't notice it I apologize.
|
|
||
| alias EctoShorts.QueryHelpers | ||
|
|
||
| defmodule MockSchema do |
There was a problem hiding this comment.
Instead of defining MockSchema and MockSchemaWithPrefix in multiple test files maybe it's time to move them out into their own files so they can cleanly be reused across tests?
There was a problem hiding this comment.
sounds good, will make that change
|
Something I'm having a hard time detecting or looking out for in this sea of changes are any breaking changes to the API that would harm any existing users. To the best of your knowledge does this PR contain anything like that? |
Not to my knowledge. However I'm going to take your feedback and split this out into smaller changes so this is easier to digest today :) |
cylkdev
force-pushed
the
feat-common-schemas
branch
from
ecef687
to
f7aa5fa
Compare
cylkdev
changed the title
|
@BobbieBarker I've applied the feedback. This is ready for another pass.
|
| |> queryable.changeset(params) | ||
| |> func.() | ||
| end | ||
| end |
There was a problem hiding this comment.
When using an abstract schema the "assoc_id" is a polymorphic reference and the name of the foreign key is different per table. This function was added so that constraints can be added to those schemas with the appropriate constraint names before the changeset is handed off to the Ecto.Repo api.
There was a problem hiding this comment.
There was a problem hiding this comment.
Not sure 100% when we should bump the version in this file, but these changes def warrant a v 2.5.0 imho.
This closes #48
Changelog
{source :: binary(), queryable :: Ecto.Queryable.t()}with the addition ofEctoShorts.CommonSchemas. The syntax can be used interchangeable in the places where an ecto schema is accepted as an argument. When the abstract table tuple is used it sets the source used for the query which takes precedence over the source defined in the schema.EctoShorts.QueryHelperswhich contains helper functions for ecto queries.EctoShorts.Actionsalong with more examples.