Allow packages to use ISO 8601 dates in their `PackageInfo.g` files (so `YYYY-MM-DD` instead of `DD/MM/YYYY`) #3355

wucas · 2019-03-19T17:11:09Z

If the date format is dd/mm/yyyy and InfoLevel InfoPackageLoading is set to at least 2 it prints a hint to change the format as was suggested in #2727. This pull request is intended to fix issue #2727.

olexandr-konovalov

This looks like a good start, I've requested some changes.

First, note that this is not everything, otherwise it would be too simple. For example, .Date is used by the package updates system - see this line and below:

https://github.com/gap-system/gap-distribution/blob/bb3123c18ca916bccb01211110fb338655ca9d6a/DistributionUpdate/PackageUpdate/PackageInfoTools.g#L339

and as it stands now, it will reject ISO 8601 dates. Second, the linked code does some checks for the validity of the date, which may be transferred here.

lib/package.gi

fingolfin · 2019-03-20T09:04:12Z

For some reason, there is a conflict in lib/package.gi -- which is a bit odd, that file was last changed in February... @wucas it seems you started your work from an old version of the GAP master branch (December 2018). I recommend that you first update the master branch of your GAP clone to the latest, then rebase you work. I'll be happy to assist with the latter, if desired.

wucas · 2019-03-20T13:24:26Z

@alex-konovalov I submitted a pull request to change this in gap-distribution too. Also ValidatePackageInfo validates the date now.

olexandr-konovalov · 2019-03-20T15:05:25Z

Thanks @wucas - I will look. A cross-reference to that PR from here would be useful though. Saves time when navigating and connects things belonging together.

codecov · 2019-03-20T15:56:35Z

Codecov Report

Merging #3355 into master will decrease coverage by 7.59%.
The diff coverage is 61.36%.

@@            Coverage Diff             @@
##           master    #3355      +/-   ##
==========================================
- Coverage   84.63%   77.03%    -7.6%     
==========================================
  Files         698      690       -8     
  Lines      345553   341652    -3901     
==========================================
- Hits       292442   263176   -29266     
- Misses      53111    78476   +25365

Impacted Files	Coverage Δ
lib/package.gi	`74.58% <61.36%> (-0.83%)`	⬇️
lib/teachm2.g	`15.38% <0%> (-84.62%)`	⬇️
lib/proto.gi	`1.03% <0%> (-82.48%)`	⬇️
lib/ctbllatt.gi	`0.81% <0%> (-79.99%)`	⬇️
lib/algliess.gi	`0.99% <0%> (-74.54%)`	⬇️
lib/attr.gi	`27.27% <0%> (-72.73%)`	⬇️
lib/ctblpope.gi	`1.77% <0%> (-72.39%)`	⬇️
lib/contfrac.gi	`30% <0%> (-67.15%)`	⬇️
src/saveload.c	`3.26% <0%> (-64.95%)`	⬇️
lib/teaching.g	`19.66% <0%> (-63.6%)`	⬇️
... and 246 more

wucas · 2019-03-20T15:58:38Z

The test in tst/testinstall/package.tst failed because the validity check of the date outputs something if the date is invalid and because the output of the format check also has a different output now. I changed this in the test file. The tests run on my computer now.

coveralls · 2019-03-20T19:33:43Z

Coverage decreased (-0.002%) to 84.369% when pulling a58cab8 on wucas:lw/allow-ISO8601-dates into 9112e3e on gap-system:master.

lib/package.gi

tst/testinstall/package.tst

lib/package.gi

wucas · 2019-03-22T09:48:02Z

@alex-konovalov you're right I overlooked the issue with the ambiguity of the dd/mm/yyyy format. I added a Mandat in the CheckDateValidity function. That way seemed most easy to me and I think with the comment it is clear what the code is supposed to do. I return true if that's the case so the "invalid Date" message is not printed.
To make sure that this is caught in the future I added a test for this.

olexandr-konovalov · 2019-03-22T22:35:11Z

It just occurred to me that this PR will not fully resolve the problem. In fact, it could even make it more difficult. If PackageInfo.g files with dates in both formats will pass validation, we will eventually have a mix of two different formats in GAPInfo.PackagesInfo, and any tool that accesses dates from these records for some purposes (report, generated content etc). will have to adapt to handle two formats (for example, the "healthcheck" report, see gap-system/gap-distribution#49).

So, I suggest that in addition to adjusting validation we should also change SetPackageInfo so that when loading package metadata it will convert all dates to ISO 8601, and will possibly accept dd/mm/yyyy dates forever for backwards compatibility.

fingolfin · 2019-03-26T22:44:59Z

Yes, please make sure that after loading, dates are in a uniform format. I'd indeed prefer ISO 8601 for that in the long run, but for the time being, we must make sure no tooling is broken. Right now, that might affect AutoDoc, which parses the Date for package manuals (see here and also the code for AUTODOC_FormatDate).

So perhaps it'd be better to "normalize" everything to the dd/mm/yyyy format; it'll then be trivial to switch over to the ISO format at any time, and also buys us time to adapt AutoDoc and whatever else might need it. And in case there is a problem, it is trivial to roll back, too.

olexandr-konovalov · 2019-04-12T22:38:36Z

@fingolfin wrote

So perhaps it'd be better to "normalize" everything to the dd/mm/yyyy format;

Ok, I thought initially about more radical change and switch to ISO 8601 now, but OK, let's do it gradually - that will allow to make a small adjustment and merge this PR then.

wucas · 2019-08-17T14:10:25Z

@fingolfin I looked at this again since I'd like to finish this up during the GAP Days next week.
So the date is stored in date which is a list of the form [dd, mm, yyyy] so isn't it "normalized"? Of course this is not the date as it is in the PackageInfo.g so should I normalize it there? And if so how can I change this file? Or am I totally misunderstanding the situation?

olexandr-konovalov · 2019-08-17T16:12:54Z

@wucas unless I miss something, I see only

Permuted(date, (1,3)); #sort such that date=[dd,mm,yyyy]

but then date is discarded. If you will inspect GAPInfo.PackagesInfo, it will still have a mix of formats - it's there where we would like to use a single format.

wucas · 2019-08-20T12:15:47Z

I have added a normalization routine. As far as I can't tell now GAPInfo.PackagesInfo.pkgname[1].Date has the format "dd/mm/yyyy" even if in the respective PackageInfo.g the date has the format "yyyy-mm-dd". @alex-konovalov does this address @fingolfin s request?

tst/testinstall/package.tst

olexandr-konovalov · 2019-09-05T08:45:36Z

This would be good to have in 4.11.0 - added it to the milestone to ensure that its stays on the radar.

DominikBernhardt · 2019-09-13T07:08:22Z

This was added to the 4.11 Milestone and @alex-konovalov requested changes. From what I understand @wucas changed code according to @wilfwilson requests. What is the status of this?

olexandr-konovalov · 2019-09-14T07:21:09Z

This waits for me to review in conjunction with gap-system/gap-distribution#78

olexandr-konovalov · 2020-03-07T23:59:24Z

Unfortunately, moving this to GAP 4.12 milestone.

fingolfin · 2020-03-19T01:02:55Z

It's rather frustrating that @wucas opened this comparatively simple change request (which is by no means to belittle the effort it took to get there) over a year ago and it is still not merged, and now has a conflict, so a rebase is necessary. I don't understand why we can't merge this right away (or could have done so in September) -- adding the ability to GAP to load packages with ISO dates before teaching the website and package distribution about ISO dates seems perfectly reasonable to me, esp. as long as we don't accept such packages for distribution.

I will attempt to rebase this now, and I really hope we can then merge it ASAP. And perhaps next week the website counterpart can be taken care of. But even if not, I think we should just merge this!

fingolfin · 2020-03-19T01:06:26Z

Just realized the PR is not over a year, but exactly one year old. Happy birthday then ;-). And I managed to rebase it, too.

fingolfin · 2020-04-21T02:04:08Z

@alex-konovalov can we merge this now?

olexandr-konovalov

I have one comment and one bug report.

lib/package.gi

olexandr-konovalov · 2020-04-21T08:38:57Z

lib/package.gi

@@ -213,7 +213,7 @@ end );
 #F  AddPackageInfo( files )
 ##
 BindGlobal( "AddPackageInfos", function( files, pkgdir, ignore )
-    local file, record, pkgname, version;
+    local file, record, pkgname, version, date, dd, mm;


So, we call ValidatePackageInfo from LoadPackage but not from AddPackageInfos. I wonder if we should do that in AddPackageInfos too...

Perhaps, but that goes beyond the purpose of this PR, doesn't it?

lib/package.gi

If the date format is dd/mm/yyyy and InfoLevel InfoPackageLoading is set to at least 2 it prints a hint to change the format. Add validity check for dates Add normalisation of date format to AddPackageInfos Update relevant testfile Fixes gap-system#2727

fingolfin · 2020-04-30T16:00:26Z

I fixed one minor whitespace typo and rebased this PR to see whether the tests pass. I also dismissed @alex-konovalov "request for changes" review, as I believe I refuted his "bug report" and consider his "concern" a feature request that goes beyond this PR.

If I hear no new concerns about this PR, I'll merge it next week.

olexandr-konovalov · 2020-04-30T19:42:26Z

Well, to do this properly, one should check:

this PR
instructions for date in https://github.com/gap-packages/example/blob/master/PackageInfo.g
Allow date format yyyy-mm-dd gap-distribution#78
https://github.com/gap-system/GapWWW (scripts that generate package-related pages).

As there is no pressing needs for this (except that this is one year old, and it would be nice to merge it), I don't plan to look at these before the middle of the next week...

olexandr-konovalov · 2021-02-20T15:45:32Z

@wucas @fingolfin thanks & let's do it.

fingolfin added gapdays2019-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2019-spring and removed gapdays2019-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2019-spring labels Mar 19, 2019

olexandr-konovalov requested changes Mar 19, 2019

View reviewed changes

lib/package.gi Outdated Show resolved Hide resolved

lib/package.gi Outdated Show resolved Hide resolved

wucas force-pushed the lw/allow-ISO8601-dates branch 2 times, most recently from 1597a99 to 4d47cfa Compare March 20, 2019 13:05

wucas force-pushed the lw/allow-ISO8601-dates branch from 4d47cfa to e9ea8fc Compare March 20, 2019 15:56

olexandr-konovalov reviewed Mar 20, 2019

View reviewed changes

lib/package.gi Show resolved Hide resolved

olexandr-konovalov mentioned this pull request Mar 20, 2019

Allow date format yyyy-mm-dd gap-system/gap-distribution#78

Open

DominikBernhardt added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Mar 21, 2019

wucas force-pushed the lw/allow-ISO8601-dates branch from e9ea8fc to 750681f Compare March 21, 2019 15:52

wucas requested a review from olexandr-konovalov March 21, 2019 22:38

olexandr-konovalov requested changes Mar 21, 2019

View reviewed changes

tst/testinstall/package.tst Outdated Show resolved Hide resolved

lib/package.gi Show resolved Hide resolved

wucas force-pushed the lw/allow-ISO8601-dates branch from 750681f to e5a30d6 Compare March 22, 2019 09:43

wucas added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Aug 20, 2019

wucas force-pushed the lw/allow-ISO8601-dates branch from 1451646 to c0b5610 Compare August 20, 2019 10:03

wucas force-pushed the lw/allow-ISO8601-dates branch from c0b5610 to e5a7083 Compare August 20, 2019 12:27

fingolfin reviewed Sep 4, 2019

View reviewed changes

tst/testinstall/package.tst Show resolved Hide resolved

olexandr-konovalov added this to the GAP 4.11.0 milestone Sep 5, 2019

wucas force-pushed the lw/allow-ISO8601-dates branch from 10113d4 to d68df66 Compare September 9, 2019 16:14

olexandr-konovalov modified the milestones: GAP 4.11.0, GAP 4.12.0 Mar 7, 2020

fingolfin force-pushed the lw/allow-ISO8601-dates branch from d68df66 to aed7d3d Compare March 19, 2020 01:06

fingolfin approved these changes Mar 19, 2020

View reviewed changes

olexandr-konovalov requested changes Apr 21, 2020

View reviewed changes

fingolfin reviewed Apr 26, 2020

View reviewed changes

lib/package.gi Outdated Show resolved Hide resolved

fingolfin force-pushed the lw/allow-ISO8601-dates branch from aed7d3d to a58cab8 Compare April 30, 2020 15:58

fingolfin requested a review from olexandr-konovalov April 30, 2020 15:58

olexandr-konovalov approved these changes Feb 20, 2021

View reviewed changes

olexandr-konovalov merged commit 5a6ed73 into gap-system:master Feb 20, 2021

fingolfin mentioned this pull request Feb 22, 2021

Don't freak out if a package has an empty or short Date #4280

Merged

wucas deleted the lw/allow-ISO8601-dates branch February 28, 2021 15:58

fingolfin changed the title ~~Allow packages to use ISO 8601 dates in their PackageInfo.g~~ Allow packages to use ISO 8601 dates in their PackageInfo.g files (so YYYY-MM-DD instead of DD/MM/YYYY) Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow packages to use ISO 8601 dates in their `PackageInfo.g` files (so `YYYY-MM-DD` instead of `DD/MM/YYYY`) #3355

Allow packages to use ISO 8601 dates in their `PackageInfo.g` files (so `YYYY-MM-DD` instead of `DD/MM/YYYY`) #3355

wucas commented Mar 19, 2019

olexandr-konovalov left a comment

fingolfin commented Mar 20, 2019

wucas commented Mar 20, 2019 •

edited

Loading

olexandr-konovalov commented Mar 20, 2019

codecov bot commented Mar 20, 2019 •

edited

Loading

wucas commented Mar 20, 2019

coveralls commented Mar 20, 2019 •

edited

Loading

wucas commented Mar 22, 2019

olexandr-konovalov commented Mar 22, 2019 •

edited

Loading

fingolfin commented Mar 26, 2019

olexandr-konovalov commented Apr 12, 2019

wucas commented Aug 17, 2019

olexandr-konovalov commented Aug 17, 2019

wucas commented Aug 20, 2019

olexandr-konovalov commented Sep 5, 2019

DominikBernhardt commented Sep 13, 2019

olexandr-konovalov commented Sep 14, 2019

olexandr-konovalov commented Mar 7, 2020

fingolfin commented Mar 19, 2020

fingolfin commented Mar 19, 2020

fingolfin commented Apr 21, 2020

olexandr-konovalov left a comment

olexandr-konovalov Apr 21, 2020

fingolfin Apr 26, 2020

fingolfin commented Apr 30, 2020

olexandr-konovalov commented Apr 30, 2020

olexandr-konovalov commented Feb 20, 2021

Allow packages to use ISO 8601 dates in their PackageInfo.g files (so YYYY-MM-DD instead of DD/MM/YYYY) #3355

Allow packages to use ISO 8601 dates in their PackageInfo.g files (so YYYY-MM-DD instead of DD/MM/YYYY) #3355

Conversation

wucas commented Mar 19, 2019

olexandr-konovalov left a comment

Choose a reason for hiding this comment

fingolfin commented Mar 20, 2019

wucas commented Mar 20, 2019 • edited Loading

olexandr-konovalov commented Mar 20, 2019

codecov bot commented Mar 20, 2019 • edited Loading

Codecov Report

wucas commented Mar 20, 2019

coveralls commented Mar 20, 2019 • edited Loading

wucas commented Mar 22, 2019

olexandr-konovalov commented Mar 22, 2019 • edited Loading

fingolfin commented Mar 26, 2019

olexandr-konovalov commented Apr 12, 2019

wucas commented Aug 17, 2019

olexandr-konovalov commented Aug 17, 2019

wucas commented Aug 20, 2019

olexandr-konovalov commented Sep 5, 2019

DominikBernhardt commented Sep 13, 2019

olexandr-konovalov commented Sep 14, 2019

olexandr-konovalov commented Mar 7, 2020

fingolfin commented Mar 19, 2020

fingolfin commented Mar 19, 2020

fingolfin commented Apr 21, 2020

olexandr-konovalov left a comment

Choose a reason for hiding this comment

olexandr-konovalov Apr 21, 2020

Choose a reason for hiding this comment

fingolfin Apr 26, 2020

Choose a reason for hiding this comment

fingolfin commented Apr 30, 2020

olexandr-konovalov commented Apr 30, 2020

olexandr-konovalov commented Feb 20, 2021

Allow packages to use ISO 8601 dates in their `PackageInfo.g` files (so `YYYY-MM-DD` instead of `DD/MM/YYYY`) #3355

Allow packages to use ISO 8601 dates in their `PackageInfo.g` files (so `YYYY-MM-DD` instead of `DD/MM/YYYY`) #3355

wucas commented Mar 20, 2019 •

edited

Loading

codecov bot commented Mar 20, 2019 •

edited

Loading

coveralls commented Mar 20, 2019 •

edited

Loading

olexandr-konovalov commented Mar 22, 2019 •

edited

Loading