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

Default DateTimeKind configuration #195

Merged
merged 4 commits into from
Aug 7, 2024

Conversation

moxplod
Copy link
Contributor

@moxplod moxplod commented Aug 4, 2024

Description

This is so we can specify a DateTimeKind to the dates that are parsed. More details in the issue.

Fixes # (issue)

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

Copy link

what-the-diff bot commented Aug 4, 2024

PR Summary

  • Documentation Update
    We've added a new explainer in the documentation (gridifyGlobalConfiguration.md) that reveals how you can variate the behavior of an important property (DefaultDateTimeKind) in the overarching GridifyGlobalConfiguration class. Essentially, this property is used to determine the type of date format that Gridify uses to understand dates. Right now, it's not set to any specific value.

  • Code Upgrade
    We've also inserted a new condition in a method in the main code file (BaseQueryBuilder.cs). This change checks if the value under review is of a date format and whether the DefaultDateTimeKind property has been given some specific value. If both are true, then the date under scrutiny is tweaked using the specified format.

  • New Property Addition
    Finally, we've added an entirely new property in the configuration file (GridifyGlobalConfiguration.cs). The property, named DefaultDateTimeKind, lets you specify the usual date format Gridify should use when trying to understand dates. At this time, it's not set to any specified format.

@moxplod
Copy link
Contributor Author

moxplod commented Aug 4, 2024

I was not quite sure how to add a test here. Did not see a specific example to test a value type - if a test is required, feel free to give pointers or any examples and I will add it to this branch.

Since this was a bug that I was facing while using this library with postgres, I have a test in my code base that I did to ensure this works.

Copy link
Owner

@alirezanet alirezanet left a comment

Choose a reason for hiding this comment

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

Hi @moxplod,

Thanks for the PR! 😊 I've added some suggestions. Last week, I was thinking that maybe supporting both DateTimeStyles and CultureInfo instead of just DateTimeKind would be more beneficial. This way, users could have more control not only over the kind but also the formatting of their dates.

I'd like to spend some time researching this further to find the best approach that addresses all date-related issues. I'd say there's a 70% chance I'll keep your changes and add DateTimeKind support to Gridify, but it's possible we might have an even better solution.

Since you're dealing directly with these issues, I'd love to hear your feedback on whether supporting both CultureInfo and DateTimeStyles would be more effective. Isn't that the way to go?

Looking forward to hearing from you!

src/Gridify/GridifyGlobalConfiguration.cs Show resolved Hide resolved
src/Gridify/Builder/BaseQueryBuilder.cs Outdated Show resolved Hide resolved
@alirezanet
Copy link
Owner

I forgot the tests, it is definitely required but please let me know if you couldn't add a few tests for these changes, I'll try to add it to your branch

@moxplod
Copy link
Contributor Author

moxplod commented Aug 7, 2024

I forgot the tests, it is definitely required but please let me know if you couldn't add a few tests for these changes, I'll try to add it to your branch

Hey - I mentioned in my commit comment, I was not sure how to add a test to verify that this is working in the Gridify solution. Essentially how do I test that the final parameters are the right DateTime Kind.

Could use your help here. I verified it on my solution and added tests to ensure that postgres dates filtering was working with Gridify to verify it. You can feel free to add a test or send me pointers to do it and I am happy to add that.

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.

Should have addressed all concerns.

@moxplod
Copy link
Contributor Author

moxplod commented Aug 7, 2024

Since you're dealing directly with these issues, I'd love to hear your feedback on whether supporting both CultureInfo and DateTimeStyles would be more effective. Isn't that the way to go?

Not sure if I know a use case on top of mind that would require CultureInfo to be specified. In Sql Server or Postgres I have not faced that requirement. I would say let's push this for now and if a future requirement of CultureInfo comes around, we can modify it then.

Copy link
Owner

@alirezanet alirezanet left a comment

Choose a reason for hiding this comment

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

LGTM 👌💐

@alirezanet alirezanet merged commit abd8c16 into alirezanet:master Aug 7, 2024
2 checks passed
@moxplod moxplod deleted the global-datetime-kind 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.

2 participants