PubMed Central: add checkbox support

[retorquere]

Fixes #1925

[retorquere]

This does not sort the checkboxes in the order they're found in the search. This can be added, but I figured I'd make the minimal changes to address the issue.

[dstillman]

If we're going to keep this rule, we should just return false for these.

[retorquere]

I'm fine with that, I just tried to fix the issue without introducing unnecessary semantic differences. Since undefined is returned implicitly, I figured making it explicit stayed closest to the original intent as expressed in the code.

[retorquere]

Can I change this particular commit without introducing a 3rd commit?

[dstillman]

Yes. Make the change, git add it, and then use git commit --fixup [hash-of-commit-you're-updating]. Then before merging, you or whoever merges this can use git rebase to squash the commit automatically (with either rebase.autosquash set to true globally, which I would recommend, or --autosquash).

https://riptutorial.com/git/example/3935/autosquash--committing-code-you-want-to-squash-during-a-rebase

(In this case you can just do the above and then force-push, since this is a small PR and easy to review.)

[retorquere]

Done the fixup and the force-push, but that still gave me a 3rd commit.

I'm not going to complain if someone else can do the rebase. rebase still terrifies me.

[dstillman]

Sorry, "the above" included the rebase. It'll be OK!

First, make sure this setting is on:

git config --global rebase.autosquash true

Then run git rebase -i 6005fa6^. That's the first commit in this PR plus ^ (which means the commit before that one).

You should see in your editor that the commits have been reordered, with the fixup commit automatically moved to after the first one. Save and close and it should be automatically merged. (Fixup means that the commit message is automatically discarded.) Then you can force push.

Technically you don't need to use -i in this case since there's nothing else to be done, but I always use it, since it's nice to see what's happening.

[retorquere]

I think that actually worked. I'll try to remember this.

[retorquere]

The last test does not pass BTW, but it doesn't pass on the current PubMed Central.js either -- complains that doc is null but scaffold does not give line numbers.

[adam3smith]

@dstillman could you clarify what you preference on merging multi-commit PRs is?

Github does have, of course, the option to just squash on merge via GUI. Should I generally do this, or is the preferences to have the submitter squash into sensible commits (e.g. one for the code fix, one for the linting-related fixes), or something else?

[adam3smith]

The last test does not pass BTW, but it doesn't pass on the current PubMed Central.js either -- complains that doc is null but scaffold does not give line numbers.

Yes, I think all tests on PDF pages currently fail. Not sure if that's fixable in the test-suite? Import into Zotero works fine.

[retorquere]

@dstillman previously explicitly requested the lint-fix commit and the functionality commit be kept separate when submitting a PR.

[adam3smith]

Absolutely, I want them separate for review, too. But it makes things easier to squash everything into one commit from the online GUI when merging.

[dstillman]

Yes, I do think it's a good idea to keep lint commits separate post-merge — that way if we have to review the commit history later we can focus on what actually changed.

For most multi-commit PRs, squashing is fine, since there's no need to keep a permanent history of every tweak that was made in response to feedback while the PR was under review. But for things that will have meaning later, I think it's worth asking the submitter to rebase at the end into logical commits. (And as shown above, git commit --fixup also makes it easy to push reviewable changes that can be automatically squashed later. I suspect GitHub's PR rebase function doesn't use autosquash, but that'd make things even easier.)

[adam3smith]

Great, thanks!