zotero / scaffold

Zotero translator creation IDE
http://www.zotero.org/support/dev/scaffold
38 stars 9 forks source link

Improve Git repo roundtrip #105

Closed dstillman closed 4 years ago

dstillman commented 5 years ago

From @retorquere in https://github.com/zotero/translators/pull/1921#issuecomment-478350241:

This is 100% your call, but personally I find the trip through scaffold cumbersome. I have to copy the translator out of the git clone into my Zotero dir, run scaffold, and then copy it back so I can run the linter. Is there a way to integrate these? Can I point Zotero to a different translators dir?

Scaffold does all sorts of things to make translator development easier, and has the potential to do even more, but I agree that the Git round-trip is annoying. It's a tricky problem to solve, though. The active translators directory is maintained by Zotero, with changes coming from both the installation files and the online translators repository, and it isn't the same as a checkout of the translators Git repo. (If you just swapped in or symlinked a Git repo, it would overwrite files, change timestamps, rename files…)

We could perhaps add the ability to specify a source directory in Scaffold (or even require it), which would cause it to automatically copy translator files from there (if they exist) when loading a translator and to there when saving. Whenever the window regained focus, it could also check the open translator in the source repo and update it in the UI if it had changed.

adam3smith commented 5 years ago

I think @zuphilip mentioned running the Zotero translators from the checked out git repo. I tried this very briefly but for some of the reasons @dstillman mentions about, it didn't work well for me.

retorquere commented 5 years ago

I realize full well it's the dissenting view, but I'm a big believer in headless automation of testing. I will always prefer a script that can be ran that will alert me to what's wrong (preferably on a schedule, or at every change) rather than having to rely on the dev to remember to update the tests in scaffold. It's great that it can be done there, and people should do it (even when it's not really frictionless -- this can also be scripted away), but even then: trust, but verify.

zuphilip commented 5 years ago

Yes, I changed Zotero's translators directory with a version from git. Before that I deactivated the automatic updates for translators in Zotero. This works fine in general, but for each update of Zotero the directory is then still updated (not sure if this is a feature or bug). However, I can easily undo this with a git stash. Moreover, to be up-to-date I need to manually do git pull.

We could perhaps add the ability to specify a source directory in Scaffold..

Yeah, that sounds good.

update the tests in scaffold [...] this can also be scripted

Yeah, why not have a script outside scaffold to update the tests. Moreover, creating a new test case is a special case of update, where no previous version is known. You could then also connect this script with git, e.g. as a pre-push hook.

dstillman commented 5 years ago

for each update of Zotero the directory is then still updated (not sure if this is a feature or bug)

That's the "overwrite files" part — it's expected.

However, I can easily undo this with a git stash.

(If you're just trying to undo the automatic changes, you'd want to use git reset --hard HEAD there, not git stash, which will store the changes until you drop them.)

@retorquere:

I realize full well it's the dissenting view, but I'm a big believer in headless automation of testing.

I'm not sure what you're saying is a dissenting view, but we obviously believe in automated testing, which we use widely. But tests also serve a purpose during development, and that's one of the many things Scaffold helps with. For many web translators, tests also have to be generated within Scaffold to be correct, because they need to run post-JS-rendering in the browser. In any case, I'm not really clear on what you're suggesting here.

retorquere commented 5 years ago

I am not owed any kind of explanation on policies set by the zotero crew. I sometimes get too invested.

dstillman commented 5 years ago

Maybe I'm misunderstanding what scaffold can do, but many import tests would fail (all that I've tried so far) when ran.

Which tests? Translator tests are run automatically every day, and the latest run shows 15/19 import tests passing. (There might be problems with some of the translator tests — you mentioned bookTitle for MODS appearing in a book item, which isn't right and does point to a problem with how the tests are generated and/or run — but that would be something to discuss elsewhere.)

from my pov it would seem worthwhile turning on the import tests

You're referring to the /import tests in translation-server? Those tests are to make sure translation-server code is working properly — they're not for translator authors. They're testing different code, they're only run when translation-server is changed, and they're tied to specific submodule commits rather than individual translator commits.

We could run the actual translator tester code for specific translators on translator commits, but that would be separate from the translation-server tests.

retorquere commented 5 years ago

Nolo contendere. I sometimes get over invested.

zuphilip commented 5 years ago

I would like to continue here the idea

We could perhaps add the ability to specify a source directory [for the translators used] in Scaffold..

How could this be achieved? I have found the following lines https://github.com/zotero/zotero/blob/master/chrome/content/zotero/xpcom/zotero.js#L964-L966

Is it possible to overwrite the getTranslatorsDirectory in Scaffold? Or would we need to extend this function in Zotero first? Would changing this already be enough?

dstillman commented 5 years ago

Yeah, I'd like for us to fix this.

Here's a possible plan: Pass an alternative Zotero.Translators to Zotero.Translate.Web()/etc. as a translatorsProvider property that would be a version of Zotero.Translators with stubs for all the get* functions. The stubs would be used throughout the translation process instead of the main Zotero.Translators functions, and they would return Zotero.Translator instances built from files in the specified directory instead of the main translator cache.

There could still be a button to save the modified translator to the current translators directory and update the internal cache (so that you could start using it right away, test in the connector, etc.), but Scaffold itself would only load files from the external source directory when specified, and the main save button would save back to the external directory, so just using Scaffold wouldn't affect your installation by default.

dstillman commented 5 years ago

We could probably also stat all files in the external directory when switching back to Scaffold, which would let you edit in an external editor if you wanted to and still test in Scaffold. Avoiding the internal cache makes things a lot easier.

zuphilip commented 5 years ago

I don't fully understand the plan, but you have much more experience with the Zotero code. Could you implement this? I am happy to test any solution or help otherwise.

dstillman commented 5 years ago

OK, I've implemented the Zotero side of this in https://github.com/zotero/zotero/pull/1711 and will try to work on the Scaffold side soon.

zuphilip commented 5 years ago

:bowing_man:

dstillman commented 4 years ago

I think this is ready, but it will need to wait until https://github.com/zotero/zotero/pull/1711 lands, which needs to wait until after 5.0.72 is released this week.

zuphilip commented 4 years ago

Thank you for that work! I am looking forward try it out. I am running Zotero beta version (currently 5.0.72-beta.3).

dstillman commented 4 years ago

You can now try this (running Scaffold from source) with the Zotero beta.

zuphilip commented 4 years ago

Wow! That works nicely (at least what I can say after my first test).

Saving was not possible, but fixed with #111 .

dstillman commented 4 years ago

OK, great. We can roll this out once Zotero 5.0.73 is released.