Generalized most of the pairs of -pr- and -commit- prefixed methods

[andrelkin]

that intended to work on the pr and commit document respectively into
common methods. The type of the document is deduced from the document
descriptor alist.

Fixed read-only buffer case that can turn in due to automatic diff-mode
on .diff extension.

Extended github-review-save-diff with
github-review-save-commit-id-in-buffer branch to controls whether to
add the commit and its parent id info to the buffer as persistent
buffer-locals.

[charignon]

What is the rationale for setting them here instead of making them buffer local? Is it because you want this to persist across session? The downside is that the user will be typically prompted to accept those on a per buffer basis which increases friction to start any review

[charignon]

@andrelkin

[andrelkin]

Is it because you want this to persist across session?

Right.

The prompting downside is just one time as the var:s can be added to
safe-local-variable-values at the first file opening.

Alternatively the variables could be placed elsewhere as plain ~"comments"
to be processed by who needs them (I obviously do).

You may have noticed already that .diff extension does not let these file local var:s be initialized at the file re-opening. The reason is the value of inhibit-local-variables-regexps which holds .diff. I am thinking to customize the github patch file extension and maybe provide the default to that like .gh-diff.

[andrelkin]

To follow up with the user-customizable file extension, it more makes sense
with a hook at the end of (github-review-save-diff). That is my preference block

+    (when github-review-save-commit-id-in-buffer
+      (when (not (null ids))
+	(set-variable 'gh-commit-id       (first  ids) t)
+	(set-variable 'gh-first-parent-id (second ids) t)

would go into my personal hook.

[charignon]

Thanks for your work, this is a nice simplification! I left one comment inline for you to take a look at.
At this point the whole stack is fairly outdated with master (alist operation + graphql API). I am planning to rebase it and update it.

[andrelkin]

Thanks for your work, this is a nice simplification! I left one comment inline for you to take a look at.
At this point the whole stack is fairly outdated with master (alist operation + graphql API). I am planning to rebase it and update it.

Thank you, Laurent! When we'll make it through, I already have another feature request in progress, to share with you hopefully sooner :-)

[andrelkin]

@charignon, howdy. After a break caused by my load at workplace, I am resuming with this request.
I see the PR has not been merged with master, perhaps not having me around to nag for that :-)?

I am trying to rebase on the up-to-date master but that's not really easy.
To ease it I went to rebase first lc--mvp-review-commit, which I won't be able to complete today and then it would wait for next time slot hopefully on the next weekend.
So I'm going to give it more efforts, but if your time permits perhaps you could beat me to that, and with quality :-)

Cheers,

Andrei

[andrelkin]

@charignon, resuming on my Apr 5 target, having time to understand how to make it the right way (git merge is not). Hope that is not interleaves with your generous merging plan (in which case just tell me to step away).