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

Change ReadPackage to print a warning if specified file cannot be read (e.g. because it is missing) #5762

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jul 3, 2024

Resolves #5745 but unfortunately can't be merged as-is because it turns out several packages read files that don't exist... But I didn't want to loose these changes, so here they are as a PR anyway. (In fact I modified this PR to only print a warning instead of just erroring out

How to proceed? Some options:

  1. we update all affected packages to not do this anymore, then once they are all updated in the package distro, we merge this but with the error turned into a warning
  2. same as 1, but we keep the warning as a warning (possibly using InfoWarning ?)
  3. some clever alternative I can't think of right now...

@ChrisJefferson
Copy link
Contributor

I think a warning is best for now. I'd be happy to merge the warning earlier (maybe 90 days? I dunno, just making up a random deadline), if packages haven't updated by then they'll start getting a harmless warning, and we still get plenty of time before the next release in case there are other issues (for example in packages which aren't distributed, if their devs or users use master)

@fingolfin fingolfin added this to the GAP 4.14.0 milestone Oct 13, 2024
@fingolfin fingolfin closed this Nov 7, 2024
@fingolfin fingolfin reopened this Nov 7, 2024
@fingolfin fingolfin changed the title ReadPackage now raises an error if file is missing / cannot be read ReadPackage now prints warning if file is missing / cannot be read Nov 7, 2024
@fingolfin fingolfin marked this pull request as ready for review November 7, 2024 08:32
lib/package.gi Outdated
else
return false;
elif error then
Print( "ReadPackage could not read <", pkgname, ">/", relpath, "\n" );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Print( "ReadPackage could not read <", pkgname, ">/", relpath, "\n" );
# TODO: turn this into an `Error` in a future GAP version
Print( "ReadPackage could not read <", pkgname, ">/", relpath, "\n" );

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should we use Info(InfoWarning, 1, "...") instead? Or Print but with some prefix like #I ?

@fingolfin
Copy link
Member Author

I think this is basically good to go now, given that (I think) no package in the distro triggers the warning anymore. To be clear: it still is just a warning, IMHO we should wait with turning this into an error for at least a full release cycle.

Would be good to get some final feedback on this.

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changed documentation promises an error, which we do not get (yet). Is this o.k.?

From the viewpoint of Oscar, it would be good to be able to suppress the message, and since Oscar already sets the level of all GAP info classes (in particular InfoWarning) to 0 by default, using Info(InfoWarning, 1, "...") would fit.

In practice, currently this would not make a difference, and eventually the warning should indeed be turned into an error, in order to make ReadPackage behave like Read. In this sense, I have no strong opinion about Info vs. Print.

@ChrisJefferson
Copy link
Contributor

I'd probably do InfoWarning, just because that feels natural, and means it can be turned off easily if it really upsets someone (Oscar, Sage) for some reason.

@fingolfin
Copy link
Member Author

Switched to InfoWarning and adjusted the documentation.

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@fingolfin fingolfin merged commit 3682796 into gap-system:master Nov 10, 2024
33 checks passed
@fingolfin fingolfin deleted the mh/ReadPackage-error branch November 10, 2024 02:21
@fingolfin fingolfin changed the title ReadPackage now prints warning if file is missing / cannot be read Change ReadPackage to print a warning if specified file cannot be read (e.g. because it is missing) Nov 13, 2024
@fingolfin fingolfin added topic: library release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements labels Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReadPackage should error when given an invalid path, not silently proceed
3 participants