Skip to content
This repository was archived by the owner on Nov 10, 2022. It is now read-only.

Allow date format yyyy-mm-dd #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wucas
Copy link

@wucas wucas commented Mar 20, 2019

This is related to gap-system/gap#3355 in gap. If there are more places to change I am happy to change them as well. It seems there was something wrong in line 348, I added some brackets to correct the if statement.

@wucas
Copy link
Author

wucas commented Mar 20, 2019

I have not tested my changes though because I don't know how to make packages and I don't have one I could tests it with. The code that checks whether a date is valid I pushed to gap as well and there I tested it.

@wucas
Copy link
Author

wucas commented Mar 20, 2019

This is also related to Issue #20

@olexandr-konovalov
Copy link
Member

Thank you! I will deal with this after gap-system/gap#3355 will stabilise.

@wucas wucas force-pushed the lw/allow-ISO8601-dates branch 2 times, most recently from f24dfcc to 753a6e4 Compare March 22, 2019 09:54
@wucas
Copy link
Author

wucas commented Mar 22, 2019

I added the check for dates in the mm/dd/yyyy format. Different to the checks in ValidatePackageInfo it will print the "invalid date" error additionally. I don't see a nice way to avoid this but I also think it's not too bad to have both errors printed since it is still clear what the problem ist.

@fingolfin
Copy link
Member

@wucas to make a package, you can use https://github.com/gap-packages/PackageManager :-)

@wucas wucas force-pushed the lw/allow-ISO8601-dates branch from 753a6e4 to 47fcb07 Compare August 20, 2019 13:37
@olexandr-konovalov
Copy link
Member

Thanks @wucas - is this now ready to be reviewed in conjunction with gap-system/gap#3355 ?

@olexandr-konovalov olexandr-konovalov self-requested a review August 23, 2019 21:49
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Sep 5, 2019
@wucas wucas force-pushed the lw/allow-ISO8601-dates branch from 47fcb07 to 6bcf1b2 Compare September 13, 2019 12:52
@wucas
Copy link
Author

wucas commented Sep 13, 2019

I use if IsBound( infon.Date ) now but as mentioned above I'am not entirely sure that this is how I should do it.

@wucas wucas force-pushed the lw/allow-ISO8601-dates branch from 6bcf1b2 to 6da9134 Compare September 22, 2019 11:14
@wucas
Copy link
Author

wucas commented Sep 22, 2019

I've made a few more changes based on @fingolfin suggestions. The idea is the following:
If no date is bound an according message is printed. Otherwise I check whether it is a string of the correct length, containing digits and -- or // at the correct positions. If this is not the case a message is printed telling the user that Date should be a string of the format dd/mm/yyyy or yyyy-mm-dd representing a date since 1999.
If this was the case then I split the string at // or -- accordingly to get a list with which I can easily check the validity of the date.
In the case of // I check whether it is plausible from the date that the format is mm/dd/yyyy and if so print a message telling the user specifically that they might switched month and day and that the format should be dd/mm/yyyy or yyyy-mm-dd.
If either of this messages is printed no other message should be printed thus I set has_error := true and do Unbind( date ).
If everything went well so far I check if the date is valid with a helper function for better readability. I the date is invalid another different message is printed.

tldr; My goal was to give the user an as specific as possible hint to what they might have done wrong. Thus four times Print now all with different messages.

@wucas wucas force-pushed the lw/allow-ISO8601-dates branch from 6da9134 to 7afd72f Compare October 1, 2019 20:19
@wucas wucas force-pushed the lw/allow-ISO8601-dates branch from 7afd72f to 500d655 Compare October 1, 2019 20:21
@fingolfin
Copy link
Member

I am completely happy with this now, thank you @wucas . So now it's up to @alex-konovalov to decide whether and when to merge this :-)

@olexandr-konovalov
Copy link
Member

For the update, gap-system/gap#3355 is now merged.

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

Successfully merging this pull request may close these issues.

3 participants