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

also omit proofs with bullets and braces #772

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

hendriktews
Copy link
Collaborator

Currently bullets and braces are classified as commands that prevent proof omission, which is wrong.

Copy link
Member

@erikmd erikmd left a comment

Choose a reason for hiding this comment

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

Very good point, thanks @hendriktews !

I proofread the PR and everything looks good to me.

Currently bullets and braces are classified as commands that prevent
proof omission, therefore proofs containing bullets or braces are
never omitted. The new tests is therefore expected to fail.
Proofs terminated with Admitted should count as failing in the
statistics, even when Coq accepts the proofs without error.

The ERT test is marked expected fail, however, this is not possible
for the tests of goal coq-proof-stat-batch-test, therefore the
expected output is wrong, such that these tests succeed.
Change `proof-check-chunks' such that Admitted proofs are reported as
failing.
@hendriktews hendriktews merged commit 0e0170f into ProofGeneral:master Jun 19, 2024
125 checks passed
@hendriktews hendriktews deleted the omit-bullets branch June 19, 2024 08:42
@erikmd
Copy link
Member

erikmd commented Jun 19, 2024

Hi @hendriktews !
sorry for the nitpick, but I believe that you used the Merge/Rebase button (?), which has 2 drawbacks documented in
https://github.com/ProofGeneral/PG/wiki/Checklist-for-testing-and-reviewing-a-PR#merging-a-pr

so AFAICT, it'd be preferable we either use Merge/Sqash or "regular" Merge (for PRs with several commits) next time.

Kind regards

@hendriktews
Copy link
Collaborator Author

yes, I prefer merge/rebase because of the linear history. Personally, I would not consider the mentioned drawbacks as a problem. We don't sign commits, the only problem remains are the PR numbers. Do you consider the PR numbers valuable?

@erikmd
Copy link
Member

erikmd commented Jun 19, 2024

Hi @hendriktews, thanks for your comment.

We don't sign commits

yes and no → we don't have a policy to sign commits manually, but github does automatically sign commits for any squash or regular merge, which is an (admittedly small) upside

do you consider the PR numbers valuable?

a bit (because it may refer to the related discussion in PR comments) but I can agree that this is subjective!

because of the linear history

Yes, the merge/rebase will produce a completely linear history. What are the upsides of this git tree from your opinion?

(just try to understand all the arguments so that we may agree it's better to either "never do a regular merge", or "always do a regular merge or squash-merge")

@erikmd
Copy link
Member

erikmd commented Jun 19, 2024

BTW, this makes me think that there might be a small risk when doing a merge/rebase:

if upstream/master has extra commits not part of the pr-branch (still without creating a conflict),

the merge/rebase creates commits that have a totally different SHA1 and git tree,
so that it is possible that none of the intermediate commits (before the tip of upstream/master after the merge-rebase) builds successfully, even if all commits of the pr-branch were fine.

(but admittedly, this is a minor risk as the most important thing is that the tip of master if fine)

@erikmd
Copy link
Member

erikmd commented Jun 19, 2024

again, sorry for the nitpick! but I'm happy to discuss whether I should do things differently (or not), from your viewpoint.

Maybe: do you think that emacs-lisp repositories are a bit specific?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants