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

Use From systematically #1348

Merged
merged 1 commit into from
Oct 23, 2024
Merged

Conversation

proux01
Copy link
Collaborator

@proux01 proux01 commented Oct 11, 2024

Motivation for this change

This is just adding From to every Require that did not already had one.

Checklist
  • [ ] added corresponding entries in CHANGELOG_UNRELEASED.md
  • [ ] added corresponding documentation in the headers

Reference: How to document

Reminder to reviewers

@affeldt-aist
Copy link
Member

affeldt-aist commented Oct 14, 2024

Until now, when inside theories, we were using From mathcomp when referring to classical files and no prefix when referring to files from theories (same reasoning for other directories). I think it was necessary to make the CI goes through and, of course, as a developer, I got used to it as a piece of information when reading headers. Your PR shows that we can put prefixes everywhere and the CI still goes through, that looks more homogeneous butiIs there an advantage in having From prefixes everywhere? (I think that I am asking because of my lack of experience with the -Q option.)

@proux01
Copy link
Collaborator Author

proux01 commented Oct 14, 2024

-Q is the same as -R except that it makes From mandatory.
The main advantage I see to the uniform use of From is to make PR creating subpackages suc as #1349 much easier, otherwise they request the addition of a lot of From everywhere, making their diff completely unreadable. A side advantage is that if Coq people add a file in the prelude/Stdlib with the same name as one of ours, this avoids the conflict.
The small drawback is that we loose the documentation that "no From" means "in theories". We could recover it by using From mathcomp.analysis instead of From mathcomp, but not sure it's a good idea and it will somewhat minimize the first advantage (when spliting sub packages).

Copy link
Member

@affeldt-aist affeldt-aist left a comment

Choose a reason for hiding this comment

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

I was a bit bothered by the fact that we lose the information about which package are in the current directory but since the developers always put them last this is really a small price to pay to get new opam packages.

@proux01
Copy link
Collaborator Author

proux01 commented Oct 21, 2024

If you prefer, we could use From mathcomp.analysis to keep that information?

@affeldt-aist
Copy link
Member

If you prefer, we could use From mathcomp.analysis to keep that information?

The discipline of keeping the local packages last is enough I think.

@affeldt-aist affeldt-aist merged commit 7b77e20 into math-comp:master Oct 23, 2024
16 checks passed
@proux01 proux01 deleted the from_mathcomp branch October 23, 2024 07:19
@affeldt-aist affeldt-aist mentioned this pull request Oct 26, 2024
2 tasks
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.

2 participants