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

move ZModulo to test suite #17258

Merged
merged 2 commits into from Sep 9, 2023
Merged

Conversation

andres-erbsen
Copy link
Contributor

@andres-erbsen andres-erbsen commented Feb 16, 2023

#16914

  • Added / updated test-suite.
  • Added changelog.

@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Feb 16, 2023
@andres-erbsen
Copy link
Contributor Author

@coqbot run full ci

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Feb 16, 2023
@andres-erbsen
Copy link
Contributor Author

elpi failure is also on master 8e03144

@proux01
Copy link
Contributor

proux01 commented Feb 20, 2023

@andres-erbsen note that the usual policy before removing something is that potential users should have been able to see a deprecation warning for at least two versions. So I'm afraid we cannot remove that now.

@andres-erbsen
Copy link
Contributor Author

andres-erbsen commented Feb 20, 2023

Are you sure this is the policy even for changes with backwards-compatible fixups? The most relevant written record I can find says

it should always be possible for a project written in Coq to be compatible with two successive major versions, features must be deprecated in one major version before they can be removed in the next, Coq developers must have a good estimate of the effort it takes to fix a project with respect to a given change, and finally and most importantly, breaking changes must be clearly documented with clear recommendations explaining how to fix a project.

(from the dissertation of @Zimmi48, https://hal.inria.fr/tel-02451322v1 page 44)

Removals based on similar reasoning have been approved and merged as well, e.g. #15268.

@Alizter
Copy link
Contributor

Alizter commented Feb 20, 2023

The fact that we cannot deprecate entire modules is a missing feature that would be very useful here.

@proux01
Copy link
Contributor

proux01 commented Feb 20, 2023

@andres-erbsen indeed I was wrong with the requirement for deprecation in two releases, one can be enough under some circumstances.
I still think that we should offer users a deprecation warning in at least one release.

@Alizter
Copy link
Contributor

Alizter commented Feb 20, 2023

Here is an issue about deprecating libraries #8032

@andres-erbsen
Copy link
Contributor Author

The deprecation was announced in the previous release here. For a library that was actually used or understood to be useful, I can see that aiming for a higher level of communication (some solution to #8032, perhaps manual) would have value, but AFAIK this file has only caused confusion so far so I don't think it is worth holding back with its removal any longer.

@Alizter
Copy link
Contributor

Alizter commented Feb 21, 2023

@andres-erbsen As it has been deprecated previously, I support you removing this.

andres-erbsen added a commit to andres-erbsen/math-classes that referenced this pull request Feb 21, 2023
@Zimmi48
Copy link
Member

Zimmi48 commented Mar 3, 2023

Given that CI reveals no issues, I also support this removal (but it should be documented in a changelog).

@Zimmi48 Zimmi48 added the needs: changelog entry This should be documented in doc/changelog. label Mar 3, 2023
@andres-erbsen andres-erbsen removed the needs: changelog entry This should be documented in doc/changelog. label Mar 6, 2023
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Mar 6, 2023
@andres-erbsen andres-erbsen marked this pull request as ready for review March 6, 2023 15:59
@andres-erbsen andres-erbsen requested review from a team as code owners March 6, 2023 15:59
@andres-erbsen andres-erbsen force-pushed the remove-Zmodulo branch 2 times, most recently from 2dd8fe9 to bb11ead Compare March 6, 2023 19:04
@andres-erbsen andres-erbsen removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Mar 8, 2023
@ppedrot ppedrot added this to the 8.18+rc1 milestone Mar 28, 2023
@ppedrot ppedrot added the kind: cleanup Code removal, deprecation, refactorings, etc. label Mar 28, 2023
@ppedrot
Copy link
Member

ppedrot commented Mar 28, 2023

AFAICT this is ready and needs an assignee. @proux01 maybe?

@proux01
Copy link
Contributor

proux01 commented Mar 28, 2023

Maybe I should clarify my position here: I still think this deserves proper deprecation warnings and I won't merge without them (I would otheriwse) but I will not oppose if anyone else think it's fine without warnings.

@ppedrot
Copy link
Member

ppedrot commented Apr 3, 2023

Either we merge this quickly, or we do deprecate the relevant definitions before the 8.18 branch. Since there is not much momentum for the first option, maybe the second should be considered?

@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label May 13, 2023
@coqbot-app coqbot-app bot added needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. and removed needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. labels May 13, 2023
@andres-erbsen andres-erbsen removed this from the 8.18+rc1 milestone May 13, 2023
@proux01 proux01 marked this pull request as draft May 13, 2023 17:18
@JasonGross
Copy link
Member

Now that 8.18 has branched, this can be merged into master, right?

@andres-erbsen andres-erbsen added this to the 8.19+rc1 milestone Aug 24, 2023
@andres-erbsen andres-erbsen marked this pull request as ready for review August 24, 2023 19:45
@andres-erbsen
Copy link
Contributor Author

Gentle ping @coq/number-maintainers

@Alizter Alizter self-assigned this Sep 8, 2023
@Alizter
Copy link
Contributor

Alizter commented Sep 8, 2023

@coqbot run full ci

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Sep 8, 2023
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Sep 9, 2023
@andres-erbsen
Copy link
Contributor Author

Full CI failures look unrelated, refman failure is fixed in latest version.

@Alizter
Copy link
Contributor

Alizter commented Sep 9, 2023

@coqbot merge now

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Sep 9, 2023

@Alizter: You cannot merge this PR because:

  • There is still a needs: full CI label.

@Alizter Alizter removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Sep 9, 2023
@Alizter
Copy link
Contributor

Alizter commented Sep 9, 2023

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 69f95b5 into coq:master Sep 9, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: cleanup Code removal, deprecation, refactorings, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants