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

New feature Global DefaultStringComparisonIsCaseInsensitive #194

Merged
merged 8 commits into from
Aug 16, 2024

Conversation

moxplod
Copy link
Contributor

@moxplod moxplod commented Aug 4, 2024

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Fixes #191 and #193

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works
  • I have made corresponding changes to the documentation
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes

@moxplod moxplod changed the title New feature - https://github.com/alirezanet/Gridify/issues/193 New feature Global DefaultStringComparisonIsCaseInsensitive Aug 4, 2024
@moxplod
Copy link
Contributor Author

moxplod commented Aug 4, 2024

For this issue - #193

src/Gridify/GridifyGlobalConfiguration.cs Outdated Show resolved Hide resolved
docs/pages/guide/gridifyGlobalConfiguration.md Outdated Show resolved Hide resolved
src/Gridify/Builder/BaseQueryBuilder.cs Outdated Show resolved Hide resolved
src/Gridify/Builder/LinqQueryBuilder.cs Outdated Show resolved Hide resolved
src/Gridify/Builder/LinqQueryBuilder.cs Outdated Show resolved Hide resolved
@moxplod
Copy link
Contributor Author

moxplod commented Aug 7, 2024

Hey, regarding your other branch and refactors. Since I do not have context, do you want to modify this PR/branch directly and take over since my code changes here are pretty minimal?

I am happy to help where I can.

@alirezanet
Copy link
Owner

Hey, regarding your other branch and refactors. Since I do not have context, do you want to modify this PR/branch directly and take over since my code changes here are pretty minimal?

I am happy to help where I can.

Sure, would be nice if you could resolve the conflicts so I can use your branch.

@alirezanet
Copy link
Owner

I was thinking maybe If the default CaseInsensitiveFiltering is set to true, it would be logical to treat the /i operator as a case-sensitive operator instead. what do you think?

@moxplod
Copy link
Contributor Author

moxplod commented Aug 7, 2024

Hey, regarding your other branch and refactors. Since I do not have context, do you want to modify this PR/branch directly and take over since my code changes here are pretty minimal?
I am happy to help where I can.

Sure, would be nice if you could resolve the conflicts so I can use your branch.

Doing that now. Thanks!

# Conflicts:
#	docs/pages/guide/gridifyGlobalConfiguration.md
#	docs/pages/guide/gridifyMapper.md
#	src/Gridify/GridifyGlobalConfiguration.cs
#	src/Gridify/GridifyMapperConfiguration.cs
@moxplod
Copy link
Contributor Author

moxplod commented Aug 7, 2024

I was thinking maybe If the default CaseInsensitiveFiltering is set to true, it would be logical to treat the /i operator as a case-sensitive operator instead. what do you think?

This makes sense to me as I am in the context of this change. We will have to explain this well in the documentation though as it may get slightly tricky for a new user.

@moxplod
Copy link
Contributor Author

moxplod commented Aug 7, 2024

Conflicts resolved @alirezanet

@alirezanet
Copy link
Owner

alirezanet commented Aug 7, 2024

This makes sense to me as I am in the context of this change. We will have to explain this well in the documentation though as it may get slightly tricky for a new user.

Yeah I agree. I'll try to update this branch with the required changes for now. Once everything is done, we can decide what changes are needed to the documentation and possibly come up with better configuration names to make it as self-explanatory as possible

@alirezanet
Copy link
Owner

alirezanet commented Aug 10, 2024

Hello @moxplod,
I've merged my updates for issue 193 into this branch. It's shaping up well, but it requires additional testing. I've also inserted a TODO in the code for future support of the Like and NotLike operators, and possibly StartWith and EndWith. Currently, I'm quite occupied, so finalizing this might be delayed. If you're available, completing the implementation for the remaining operators and their testing would be our subsequent move.
also we need to test contains methods for non-string properties to make sure it is still working.

Repository owner deleted a comment from what-the-diff bot Aug 10, 2024
@alirezanet alirezanet added WTD and removed WTD labels Aug 10, 2024
@moxplod
Copy link
Contributor Author

moxplod commented Aug 13, 2024

Hey, @alirezanet, I got a pretty busy work week as well unfortunately.
I am not very fluent with the expression creation and the syntaxes there, so may take some time to understand the changes here :)

But, Thanks for this. I will try and get to it this week.

@alirezanet
Copy link
Owner

Hi @moxplod,
I think this is ready to merge, I hope I didn't miss anything... would be nice if you do a double check and review it again if you were happy with the new changes we can merge it

@moxplod
Copy link
Contributor Author

moxplod commented Aug 16, 2024

This looks great! Thanks for getting it across the finish line.

Copy link
Contributor Author

@moxplod moxplod left a comment

Choose a reason for hiding this comment

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

Looks great

@alirezanet alirezanet merged commit 0f4b323 into alirezanet:master Aug 16, 2024
3 checks passed
@moxplod moxplod deleted the global-case-insensitive branch August 29, 2024 18:49
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.

Global configuration for case insensitive string search
2 participants