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

Generic Filters #384

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

nkooman
Copy link

@nkooman nkooman commented Oct 17, 2023

Motivation

Fixes #119

I did not add the specific DateTime and decimal constructors. I have also kept the original method signatures for all of the filters for backward compatibility.
The issue (#119) calls out that all filters should implement DateTime and decimal constructors, then utilize a new constructor in Filter to accept objects as values. The original issue (#27), has a proposed solution in such a way; however, doing so defeats the purpose of creating a generic filter in the first place. Since, if we were to go the object route, all values would get converted to a string, removing the need for a generic and for GetQueryStringParameter to handle types in a switch.
Instead, I opted to go the route of the generic filter and implemented a switch statement to interpret string, DateTime, and numeric types (int, decimal, float, etc.). The downside here is that we lose the specific constructors for DateTime and decimal values.

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added
  • Tests are passing
  • Docs have been updated (if applicable)
  • Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

Create queries using the new filters and confirm that results are correctly filtered.

@nkooman nkooman requested review from pokornyd and a team as code owners October 17, 2023 23:32
@nkooman
Copy link
Author

nkooman commented Oct 30, 2023

I've been looking into some of the other issues and think it would be better if we combined #56, #163, and #384 into a single issue. Then we can refactor the filters so that they support a Fluent API structure.
@pokornyd Looking for some thoughts on this idea before I close this pull request.

@pokornyd
Copy link
Member

pokornyd commented Nov 7, 2023

Hello @nkooman and thank you for your contribution, forgive the rather late response. I'll review your proposed changes, as well as the issues you suggested for merging with this PR and get back to you with any comments, thanks for your patience.

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.

Add DateTime and numeric constructors to filters
3 participants