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

GAP doesn't report duplicated rec keys #5876

Open
ChrisJefferson opened this issue Dec 19, 2024 · 1 comment
Open

GAP doesn't report duplicated rec keys #5876

ChrisJefferson opened this issue Dec 19, 2024 · 1 comment

Comments

@ChrisJefferson
Copy link
Contributor

I had a bug in my own code, which I eventually tracked down to something like:

rec(a := 1, b := 2, a := 3)

which ends up being equivalent to rec(b := 2, a : =3).

Now, this is clearly my fault, but (I'd argue) when someone writes a rec out like this explictly, they probably didn't mean to duplicate the keys!

I quickly hacked something together to see if any code exists like this in GAP. I didn't find any in the kernel, but there are a bunch in PackageInfo.gs. This makes sense, because those are fairly long records, written by hand. There are also more in packages. From my initial check, none of them seem to be a bug, but we could go through and report them all.

I could tidy up my for reporting these issues, the main problem is what should it do? Do we want to make this a warning? Should it be possible to disable that warning? Probably entering the break loop is most useful (as then you see where the issue is), but you wouldn't want that always turned on, as at the moment GAP wouldn't load many packages with that turned on.

Here, from my hacked up code, is all the places where this issue current occurs while loading GAP + packages, it shows keys and values. I have checked and these are all in PackageInfo.g files, most are obvious, for some dates you have to search for that date to find which package it is mentioned in.

#W Duplicate record member:
#w Key: PackageWWWHome
#W Value:"https://gap-packages.github.io/OrbitalGraphs"
#W Duplicate record member:
#w Key: Date
#W Value:"19/06/2023"
#W Duplicate record member:
#w Key: PackageWWWHome
#W Value:"https://gap-packages.github.io/AutoDoc"
#W Duplicate record member:
#w Key: Date
#W Value:"27/08/2024"
#W Duplicate record member:
#w Key: TestFile
#W Value:"tst/testall.g"
#W Duplicate record member:
#w Key: PackageWWWHome
#W Value:"https://gap-packages.github.io/float/"
#W Duplicate record member:
#w Key: TestFile
#W Value:"tst/testall.g"
#W Duplicate record member:
#w Key: Date
#W Value:"30/09/2022"
#W Duplicate record member:
#w Key: README_URL
#W Value:"https://gap-packages.github.io/tomlib/README.md"
#W Duplicate record member:
#w Key: PackageInfoURL
#W Value:"https://gap-packages.github.io/tomlib/PackageInfo.g"
#W Duplicate record member:
#w Key: Date
#W Value:"13/07/2022"
#W Duplicate record member:
#w Key: PackageWWWHome
#W Value:"https://gap-packages.github.io/walrus"
@fingolfin
Copy link
Member

Yes I think this would be a useful warning. Of course it then should print out more, specifically, in which file (and ideally in which line) the issue was encountered.

In light of a growing number of such warnings, I'd also like us to

  • add a "strict" mode in which these warning are treated as errors (e.g. --strict or --warnings=error or so)
  • add a way to disable them (e.g. --warnings=off?)

Strict mode would be useful for CI: once a package fixed all these warnings, it can configure its CI to run in strict mode, to make sure no regressions creep in.

See also #1191 for more ideas for things that we could warn about / that strict mode could turn into errors.

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

No branches or pull requests

2 participants