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

Several small additions/tweaks #14

Closed
wants to merge 3 commits into from

Conversation

mayel
Copy link

@mayel mayel commented Feb 26, 2022

As discussed here's a PR with some changes I made while using EctoShorts for Bonfire :)

  • Added support for passing ecto dynamic filters
  • Added support for passing functions to use for custom filtering
  • Added EctoShorts.filter as a shortcut to EctoShorts.CommonFilters.convert_params_to_filter
  • Added a default order_by field and direction for queries
  • Added :id to common filters
  • Added the readme to generated docs, which becomes the main source of documentation, with additional docs inline specific modules and functions

@mayel mayel changed the title Several small additions additions/tweaks Several small additions/tweaks Feb 26, 2022
Copy link
Owner

@MikaAK MikaAK left a comment

Choose a reason for hiding this comment

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

Hey there, thanks for the contribution, I think however there are a few issues to take care of, primarily I think we would only want to accept the usage of dynamic filters

#### WIP
- Added support for passing ecto `dynamic` filters
- Added support for passing functions to use for custom filtering
- Added `EctoShorts.filter` as a shortcut to `CommonFilters.convert_params_to_filter`
Copy link
Owner

Choose a reason for hiding this comment

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

Think this is redundant and wouldn't add this in

- Added support for passing ecto `dynamic` filters
- Added support for passing functions to use for custom filtering
- Added `EctoShorts.filter` as a shortcut to `CommonFilters.convert_params_to_filter`
- Added a default order_by field and direction for queries
Copy link
Owner

@MikaAK MikaAK Mar 14, 2022

Choose a reason for hiding this comment

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

Default ordering shouldn't exist, this can cause performance issues, also the direction has a default naturally, the order param was actually removed because of this, instead you can now specify %{order_by: :field} or %{order_by: [asc: :field]}

- Added support for passing functions to use for custom filtering
- Added `EctoShorts.filter` as a shortcut to `CommonFilters.convert_params_to_filter`
- Added a default order_by field and direction for queries
- Added `:id` to common filters
Copy link
Owner

Choose a reason for hiding this comment

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

This shouldn't be necessary, :id if a valid field on the schema will get run through the schema filters and be put in

MIT

- Copyright 2020 Mika Kalathil
- Copyright 2021 Bonfire contributors
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure about this

@@ -1,5 +1,13 @@
## Changelog

#### WIP
- Added support for passing ecto `dynamic` filters
- Added support for passing functions to use for custom filtering
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think you should do this, ultimately this is why the first parameter is a query and can be passed in, because you can add custom filtering there

@MikaAK
Copy link
Owner

MikaAK commented Jun 16, 2022

Going to close this as it's gotten pretty stale, feel free to re-open if needed!

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