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

fix DateOnly has current date return 'today' #1278

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

AJacksonCT
Copy link

fixes #1186 DateOnly has current date return 'today'

Here is a checklist you should tick through before submitting a pull request:

[x ] Implementation is clean
[ x] Code adheres to the existing coding standards; e.g. no curlies for one-line blocks, no redundant empty lines between methods or code blocks, spaces rather than tabs, etc.
[x ] No Code Analysis warnings
[x ] There is proper unit test coverage
If the code is copied from StackOverflow (or a blog or OSS) full disclosure is included. That includes required license files and/or file headers explaining where the code came from with proper attribution
[x ] There are very few or no comments (because comments shouldn't be needed if you write clean code)
[x ] Xml documentation is added/updated for the addition/change
[x ] Your PR is (re)based on top of the latest commits from the main branch (more info below)
[x ] Link to the issue(s) you're fixing from your PR description. Use fixes #
[x ] Readme is updated if you change an existing feature or add a new one
[x ] Run either build.cmd or build.ps1 and ensure there are no test failures

@SimonCropp SimonCropp changed the title fixes https://github.com/Humanizr/Humanizer/issues/1186 DateOnly has current date return 'today' fix DateOnly has current date return 'today' Feb 16, 2024
@hazzik
Copy link
Member

hazzik commented Feb 16, 2024

Imho, the changes to DateTime behaviour are not really expected.

@SimonCropp
Copy link
Collaborator

i agree with @hazzik. closing this one.

@ AJacksonCT if you still want this approach, can you re submit as an opt in approach

@SimonCropp SimonCropp closed this Feb 16, 2024
@AJacksonCT
Copy link
Author

Not quite sure what happened here. This project had a call out for help. I submitted a PR for the work you were looking for. I have no concern whether or not this goes forward.

@SimonCropp SimonCropp reopened this Feb 16, 2024
@SimonCropp
Copy link
Collaborator

ok after re-reading the issue more closely i think i was mistaken in closing this. since dateonly has no time component, it is incorrect to use the phrasing "from now" since that incorrectly implies an instance in time. in english a dateonly for the current date is literally "today".

@hazzik thoughts?

@hazzik
Copy link
Member

hazzik commented Feb 17, 2024

@SimonCropp @AJacksonCT I have no objection to fix DateOnly humanization behavior and return “from today” as it makes more sense than “from now” in DateOnly context. However, as the original issue mentioned DateOnly uses DateTime algorithm which makes it tricky. And it seems that this PR changes the algorithm for not only DateOnly, but also for DateTime.

I have not seen the discussion around changing behavior for DateTime for days and over to use “from today” instead of “from now”.

@SimonCropp
Copy link
Collaborator

@AJacksonCT thoughts?

@clairernovotny clairernovotny added this to the v3.0 milestone Mar 8, 2024
@SimonCropp
Copy link
Collaborator

@clairernovotny do u want to proceed with this?

@SimonCropp
Copy link
Collaborator

@AJacksonCT can you rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DateOnly has current date return 'today'
4 participants