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

Support date shortcuts; see issue #13 #21

Merged
merged 24 commits into from
Dec 5, 2024
Merged

Conversation

kaat0
Copy link
Contributor

@kaat0 kaat0 commented Nov 26, 2024

Tests are currently missing and help is welcome.

src/Overpass.jl Show resolved Hide resolved
@gwehrle
Copy link
Owner

gwehrle commented Nov 27, 2024

I fixed the tests 🎉
It was pretty hard to find out that adding TimeZones.jl modifies parsing of Date.jl library 😮‍💨

I'll add some tests for the new function soon.

@gwehrle
Copy link
Owner

gwehrle commented Nov 27, 2024

I just saw that in Overpass turbo time calculation is done with constants of seconds. This means for durations like months or years this could lead to different behaviour. I still would stick to the current behaviour in this PR, as this is the correct way. But it should be added to documentation.

https://github.com/tyrasd/overpass-turbo/blob/0ff477e87d947645a6f70a4d408edf42d1166306/js/shortcuts.ts#L44-L74

@gwehrle
Copy link
Owner

gwehrle commented Dec 4, 2024

All tests are done. For me this feature is ready to merge.
What do you think @kaat0 ? Is there something missing?

@gwehrle
Copy link
Owner

gwehrle commented Dec 5, 2024

Okay there are still two todos:

  • remove Timezones dependency, instead use Dates now(UTC)
  • remove Mocking dependency, as test code doesn't belong in production code

@gwehrle gwehrle merged commit 500a8fe into gwehrle:main Dec 5, 2024
20 checks passed
@gwehrle
Copy link
Owner

gwehrle commented Dec 5, 2024

@kaat0 Thank you a lot!

@kaat0 kaat0 deleted the date-shortcut branch December 5, 2024 21:59
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