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

fix #596 - outdated documentation for setup coq-prog-xxx. #598

Merged
merged 1 commit into from Sep 13, 2021

Conversation

Matafou
Copy link
Contributor

@Matafou Matafou commented Sep 10, 2021

Fix #596

I did a refresh on this part of the documentation.
I propose to merge this before adapting maybe according to #590.

@Matafou
Copy link
Contributor Author

Matafou commented Sep 12, 2021

I will merg e this soon if noone objects.

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.

Thanks @Matafou; I did a quick review and left very minor suggestions

doc/ProofGeneral.texi Outdated Show resolved Hide resolved
Comment on lines -4220 to -4224
Coq Proof General has a special experimental feature called "Holes"
which makes use of the abbreviation mechanism and includes a large list
of command abbreviations. @xref{Holes feature}, for details. With other
provers, you may use the standard Emacs commands above to set up your
own abbreviation tables.
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want to remove this documentation from now on? 😉
but maybe you'd have plans to port part of the features of Holes to the same "elisp backend" as that of company-coq?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I made a small poll on Zulip and it appear I was the only user of this feature.
I have tested yasnippet snippets for coq and it works quite well and it does not interfere with company coc completions (even if it also uses yasnippet). There are a few editing tricks that I will miss but nevermind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More precisely I plan to

  1. remove everything from the holes library shortly.
  2. document and/or propose a set of user-customized abbreviations (unless this subsumed by company-coq maybe)
  3. one day maybe try to reimplement the copy-paste-in-the-holes thing, even if nobody seems to be willing to try it...

doc/ProofGeneral.texi Outdated Show resolved Hide resolved
doc/ProofGeneral.texi Outdated Show resolved Hide resolved
@erikmd
Copy link
Member

erikmd commented Sep 12, 2021

So feel free @Matafou to merge this when you see fit.

BTW, small remark: it happens there are 3 PR merging strategies in GitHub (Merge, Merge/Squash, Merge/Rebase),
and I believe you have recently used the Merge/Rebase one? (maybe)

It seems actually that Merge/Rebase has two drawbacks:

  • there is no mention anywhere of the PR number in the resulting commits messages;
  • and the resulting commits are not GPG-signed (cf. the missing Verified slug on those commits).

However, Merge and Merge/Squash are very fine − Merge/Squash being especially fine if the PR contains only one commit I guess.

So, maybe for merging upcoming PRs we should only use Merge or Merge/Squash 😅

(I guess I could copy this suggestion in the wiki later on? Cc @cpitclaudel @hendriktews @Matafou @monnier FYI)

@erikmd
Copy link
Member

erikmd commented Sep 12, 2021

(I guess I could copy this suggestion in the wiki later on? Cc @cpitclaudel @hendriktews @Matafou @monnier FYI)

Done FYI: https://github.com/ProofGeneral/PG/wiki/Checklist-for-testing-and-reviewing-a-PR#merging-a-pr

@Matafou
Copy link
Contributor Author

Matafou commented Sep 13, 2021

Thanks @erikmd.

@Matafou
Copy link
Contributor Author

Matafou commented Sep 13, 2021

BTW @erikmd sorry for the rebase, this is the default "merge" button behaviour.

@Matafou Matafou merged commit e2b4227 into ProofGeneral:master Sep 13, 2021
@erikmd
Copy link
Member

erikmd commented Sep 13, 2021

No worries for this time ;)
Actually it's not the default action for me... so I believe the "default" action is just the last one used by the logged-in user. So to change it we need to use the drop-down list I guess.

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.

Broken (possibly outdated?) setup instructions regarding coq-prog-name
2 participants