Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upFixes most MODS import tests #1917
Conversation
This comment has been minimized.
This comment has been minimized.
Simon isn't involved with Zotero development anymore, so if you have a question you should ask it here and hopefully someone will know. |
This comment has been minimized.
This comment has been minimized.
As we've discussed, lint changes should be in separate commits so they can be reviewed quickly, separately from substantive changes. Can you fix that and force-push? |
This comment has been minimized.
This comment has been minimized.
So if I understand correctly - you prefer changes to translators in one PR, but one PR with two commits - one that does the lint changes, one that does functional changes? |
This comment has been minimized.
This comment has been minimized.
That's right. |
This comment has been minimized.
This comment has been minimized.
Alright, I didn't get that from the earlier discussion, but I'll do that Sunday. |
retorquere
force-pushed the
retorquere:MODS-import-test
branch
from
b270208
to
69b6e48
Mar 30, 2019
This comment has been minimized.
This comment has been minimized.
OK, can you look at the lint reports first before I stack the other changes on to them? |
This comment has been minimized.
This comment has been minimized.
It won't necessarily be me reviewing this, but there's no need to do this in stages. Git makes it easy to add changes to previous commits: https://stackoverflow.com/a/1186549 or https://fle.github.io/git-tip-keep-your-branch-clean-with-fixup-and-autosquash.html |
This comment has been minimized.
This comment has been minimized.
As much as I love git, rebase still terrifies me. If it's OK with you (or whomever might end up reviewing this), I'd prefer to wait and stack the commits using regular add. |
This comment has been minimized.
This comment has been minimized.
@retorquere -- I had a look and all the changes look fine. I think the remaining linting error should disappear on your next push as we added |
This comment has been minimized.
This comment has been minimized.
If you have substantive changes you want to make -- I have this one that I'd want to also include: adam3smith@783ac38 |
This comment has been minimized.
This comment has been minimized.
I was going to address the import tests in this PR, but my understanding of how they're going to be used has changed -- I thought the translation-server would be the test runner for the At this stage, it may be easier to just close this PR and work from adam3smith@783ac38 |
This comment has been minimized.
This comment has been minimized.
One way or another, please be sure to rebase to remove the merge commit before merging. Again, there should almost never be merge commits in PRs, and certainly never merges back from the master branch. |
adam3smith
referenced this pull request
Apr 12, 2019
Merged
MODS: Better page ranges (and linting fix) #1928
This comment has been minimized.
This comment has been minimized.
Closed via #1928 @retorquere -- I'm not sure what you were referring to with the import tests. The way we create and run them is in Scaffold, and all tests pass in the current MODS translator, so nothing to fix there, but I may be misunderstanding. |
adam3smith
closed this
Apr 12, 2019
This comment has been minimized.
This comment has been minimized.
The misunderstanding was from me, and you clarified it earlier. I knew scaffold was a way to run the tests but I didn't realize it was the way to run the tests. I thought I was working towards a way to have the import tests be runnable automatically so that they could, in principle, be ran in CI (so as to prevent something like my gaffe with the BibTeX translator happening again), but as you pointed out earlier, that's not strictly what the tests on translation-server are for. |
This comment has been minimized.
This comment has been minimized.
You're still confusing the translation-server tests and the translator tests. As I've noted, translator tests are already run automatically, and they could be run via CI in this repo. |
This comment has been minimized.
This comment has been minimized.
Which is exactly my point. I was confusing them, and you corrected that. |
retorquere commentedMar 28, 2019
One still remains but I don't know MODS well enough to decide what to do; I've sent a message to Simon to ask.