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

refactor: Replace CurrentDateTime with DateForValidation and use LocalDate #1636

Merged
merged 5 commits into from
Jan 12, 2024

Conversation

bradyhunsaker
Copy link
Contributor

Summary:

Preparation to make PR #1628 for issue #1292 clearer.

Several rules compare dates in the feed to a reference date, which is usually today. The object that stores the date is called CurrentDateTime and it wraps a ZonedDateTime object. But only the date is ever used (not the time), and the date can be overridden in tests to use specific dates other than today.

This refactor renames the object DateForValidation and wraps a LocalDate object instead. This simplifies the code in several places and will allow PR #1628 to be clearer.

Expected behavior:

No change in behavior. Internally a LocalDate will be used instead of a ZonedDateTime.

Please make sure these boxes are checked before submitting your pull request - thanks!

  • [X ] Run the unit tests with gradle test to make sure you didn't break anything
  • [X ] Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • [X ] Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@jcpitre
Copy link
Contributor

jcpitre commented Jan 10, 2024

Let's complicate things a bit ;)
Take a look at #1637
How does this affect this PR?

@bradyhunsaker
Copy link
Contributor Author

It's easy to support a timezone with the changes in this PR (and also with the existing code).

The date is set with LocalDate.now(), but there is another form of the method now() that takes a timezone (ZoneID). Since timezone is required in the feed, it seems straightforward. I expect the only challenge there will be dealing with improper timezone entries.

But that should be in a separate PR from this refactor, since it is a change in behavior.

Copy link
Contributor

@jcpitre jcpitre left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks

@bradyhunsaker
Copy link
Contributor Author

I tried to add in updates (not mine) that had been made to the "master" branch, but I may have done it incorrectly.

Could someone let me know if I need to change anything before this can be merged?

@davidgamez
Copy link
Member

I tried to add in updates (not mine) that had been made to the "master" branch, but I may have done it incorrectly.

Could someone let me know if I need to change anything before this can be merged?

Hi, @bradyhunsaker; it looks like the code formatting check is failing on your branch. As soon as fixed, we can merge it.

@bradyhunsaker
Copy link
Contributor Author

I just pushed a commit that was the result of running the Java formatter. I should have run it before my original push.

@davidgamez davidgamez merged commit 6f9aec7 into MobilityData:master Jan 12, 2024
333 checks passed
@bradyhunsaker bradyhunsaker deleted the date-for-validation branch January 19, 2024 00:27
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.

3 participants