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 time support #68

Merged
merged 3 commits into from
Nov 5, 2021
Merged

Fix time support #68

merged 3 commits into from
Nov 5, 2021

Conversation

diegodlh
Copy link
Contributor

I think these commits fix some bugs with time values. Let me know if I should send three separate PRs.

  • 3400deb: fixes validation error because time was not considered as a rich value
  • 26cf918: fixes getTimeStringBase error for times with leading plus sign
  • 431f206: ISO validation was failing with valid low-precision times such as 2021-00-00 or 2021-01-00.

BTW, regarding 431f206, you write "Only trying to parse 5-digit years or below". Shouldn't 5-digit years fail? Shouldn't that be 4-digits only?

@maxlath
Copy link
Owner

maxlath commented Jun 1, 2021

Ideally, we should have unit tests to avoid future regressions on those different points: would you like to try adding them or should I do it?

On the 5-digit year point: while most cases are limited to 4-digit years, Wikibase supports date far in the past and the future (example), so this lib should probably do what it can to support it

@diegodlh
Copy link
Contributor Author

diegodlh commented Jun 3, 2021

Ideally, we should have unit tests to avoid future regressions on those different points: would you like to try adding them or should I do it?

Actually, I don't know how to do that. I have to learn, because I'd like to add them to my current and future projects, but I haven't had the time to do that yet :'(

On the 5-digit year point: while most cases are limited to 4-digit years, Wikibase supports date far in the past and the future (example), so this lib should probably do what it can to support it

Sure! But the point I'm referring to is the check you make by converting the time to an ISO string. I understand 5-digit years would fail there (you're already skipping this check for years longer than 5 digits).

@maxlath
Copy link
Owner

maxlath commented Jun 15, 2021

Learning to use automated tests is definitely a good investment :D I tried to improve the setup documentation to help you give it a try: https://github.com/maxlath/wikibase-edit/blob/master/docs/development_setup.md#development-setup Feel welcome to ask if you get stuck (or would prefer to pass this time ;))

@diegodlh
Copy link
Contributor Author

Hi, Max! Thanks for improving the documentation to help me giving it a try! I definitely would like to use this opportunity to learn! More so with a mature project like this, and with a more experienced programmer like you around to help. Although I'm not sure when I will be able to do it. If it can wait a bit more, I'd be thrilled to do it as soon as possible. But feel free to do it yourself if you think it can't wait any longer. Thank you so much!

@maxlath
Copy link
Owner

maxlath commented Jun 24, 2021

no hurry on my side :)

diegodlh added a commit to diegodlh/wikibase-edit that referenced this pull request Jul 12, 2021
Found a couple typos while reading this to work on adding test cases for maxlath#68
@diegodlh
Copy link
Contributor Author

Hi, @maxlath! I've added some unit tests for the changes introduced by my commits above, as suggested. I've squashed the new commits into the old ones and forced push to keep the history cleaner. When you can, please give it a look and tell me what you think.

Sure! But the point I'm referring to is the check you make by converting the time to an ISO string. I understand 5-digit years would fail there (you're already skipping this check for years longer than 5 digits).

Regarding this, I'll open a separate issue, to keep things clearer.

Thanks!!

@maxlath maxlath merged commit 722025e into maxlath:main Nov 5, 2021
@maxlath
Copy link
Owner

maxlath commented Nov 5, 2021

nice! published in v4.14.3 🎉

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.

None yet

2 participants