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

Require NsatzTactic: nsatz support for Z and Q #12861

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

JasonGross
Copy link
Member

The purpose of NsatzTactic is to allow using nsatz without the
dependency on real axioms. So we declare the instances for Z and Q
in that file, so that users don't have to re-create them.

Fixes #12860

Kind: bug fix / enhancement

Fixes / closes #12860

@JasonGross JasonGross added the part: standard library The standard library stdlib. label Aug 19, 2020
@JasonGross JasonGross added this to the 8.13+beta1 milestone Aug 19, 2020
@JasonGross JasonGross requested a review from a team as a code owner August 19, 2020 17:10
@coqbot
Copy link
Contributor

coqbot commented Aug 19, 2020

For your complete information, the job test-suite:4.11+trunk+dune in allow failure mode has failed for commit f67d6e6: Require NsatzTactic: nsatz support for Z and Q

Copy link
Contributor

@thery thery left a comment

Choose a reason for hiding this comment

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

Looks good but maybe there should be a footnote in the doc https://coq.inria.fr/refman/addendum/nsatz.html that tells what is going on.

@JasonGross JasonGross requested a review from a team as a code owner August 19, 2020 19:23
@JasonGross
Copy link
Member Author

Documentation updated; is this approximately what you were thinking of?

@coqbot
Copy link
Contributor

coqbot commented Aug 19, 2020

For your complete information, the job test-suite:4.11+trunk+dune in allow failure mode has failed for commit 021f02d: Update nsatz documentation, as requested

Copy link
Contributor

@thery thery left a comment

Choose a reason for hiding this comment

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

fine with me

Copy link
Member

@jfehrle jfehrle left a comment

Choose a reason for hiding this comment

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

Only looked at the doc, a few wording suggestions, please change the ones that make sense to you.

doc/sphinx/addendum/nsatz.rst Outdated Show resolved Hide resolved
doc/sphinx/addendum/nsatz.rst Outdated Show resolved Hide resolved
@coqbot
Copy link
Contributor

coqbot commented Aug 19, 2020

For your complete information, the job test-suite:4.11+trunk+dune in allow failure mode has failed for commit 9c12116: Apply suggestions from code review

@SkySkimmer SkySkimmer added the needs: squashing Some commits should be squashed together. label Aug 25, 2020
@SkySkimmer
Copy link
Contributor

@JasonGross this should probably be squashed
@coq/nsatz-maintainers please assign

@ppedrot ppedrot self-assigned this Aug 25, 2020
@ppedrot
Copy link
Member

ppedrot commented Aug 25, 2020

Squashing the documentation commits looks like a good idea indeed, but it's not mandatory either. I'll merge soon anyways.

@jfehrle
Copy link
Member

jfehrle commented Aug 25, 2020

Please squash before the merge. The individual commits are not interesting, more noise in the git history.

The purpose of `NsatzTactic` is to allow using `nsatz` without the
dependency on real axioms. So we declare the instances for `Z` and `Q`
in that file, so that users don't have to re-create them.

Fixes coq#12860
@JasonGross
Copy link
Member Author

I've squashed all the commits.

@ppedrot ppedrot added kind: fix This fixes a bug or incorrect documentation. and removed needs: squashing Some commits should be squashed together. labels Aug 25, 2020
@ppedrot ppedrot merged commit e01f9df into coq:master Aug 25, 2020
@JasonGross JasonGross deleted the nsatz-tactic-instances branch August 26, 2020 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: fix This fixes a bug or incorrect documentation. part: standard library The standard library stdlib.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Require Import NsatzTactic should allow nsatz to work over Z and Q
7 participants