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

Clean up Regnum phylorefs #89

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Clean up Regnum phylorefs #89

wants to merge 24 commits into from

Conversation

gaurav
Copy link
Member

@gaurav gaurav commented Jul 2, 2021

This PR cleans up Regnum phylorefs in several ways:

  1. Renamed files from REGNUM_* to CLADO_* and increased the number of digits to seven.
  2. Added some scripts to test and verify these Phyx files.
  3. Used these scripts to fix specifier authorities that were not loaded correctly.
  4. Incorporated some of the phylogenies prepared by Anna Becker.
  5. Moved Phylonym Phyx files out of the encrypted/ directory so that they will no longer be encrypted.
  6. Add a GitHub Action for testing these phyloreferences.

The tests are in no way fully implemented or comprehensive yet, but are sophisticated enough to note which phyloreferences include phylogenies.

Hilmar: this is a huge PR, but I don't think it makes sense to make it smaller -- it includes a bunch of changes on the encrypted files, followed by the de-encryption of the files, followed by tweaking the tests so they would pass. I could split this into two or three PRs, but if so, the first PR would just end with a bunch of encrypted files :). So I think this is better. Let me know if you disagree!

@gaurav gaurav force-pushed the fix-specifier-authorities branch from 99b5364 to 8e18aff Compare July 2, 2021 04:56
@gaurav gaurav marked this pull request as ready for review July 13, 2021 00:09
@gaurav gaurav requested a review from hlapp July 13, 2021 00:09
@hlapp
Copy link
Member

hlapp commented Jul 13, 2021

Well, it sounds like there are two types of changes wrapped into one. By two types I mean a set of changes that does not lend itself to code review, and those that do. The overwhelming bulk (by "lines changed") seems to fall into the former category, suggesting that there is a small enough set in the latter category to still enable review.

As it is, this PR is unreviewable, so if we keep it we shouldn't pretend it was ever reviewed. It sounds to me though that the changes that don't stand to benefit from review (decrypting files, for example) can be separated out, even if they would necessarily have to be accompanied by some code changes (in the tests) too. But there seem to be additional code changes (items 2, 3, and 6 in your list) that are separable changes of code that seem reviewable and would benefit from having been reviewed.

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

Successfully merging this pull request may close these issues.

2 participants