translation changes not getting marked

[lmccart]

Currently, running grunt after changing src/data/en.yml before committing will mark the translation file that needs to be updated whenever it detect changes. However, it needs to run the build step before committing. I've noticed that this often doesn't happen and then these changes are not marked. I'm wondering if it's possible to use a git precommit hook to automatically runt grunt?

cc @limzykenneth

[limzykenneth]

I have considered that as well but with precommit hook, it is possible to commit the changes made by the tracking script?

[Zalastax]

It is possible to do it, but I am personally scared of doing so (I want to stage things manually). My preferred solution would be to run the script in a precommit hook and abort the commit if any files are different afterwards. I don't know how to check if files have changed though.

[limzykenneth]

@Zalastax I think aborting the commit is good as long as it can also send a clear message about needing to commit changes in the tracking file.

Checking the files currently are done with nodegit package by diffing src/data/en.yml currently with HEAD and extracting the diff lines from the result, it is kinda hacky but it fulfills my requirement for the system.

[Zalastax]

@limzykenneth it's very easy to send a clear message 😃, for example see what happens if you commit ill-formatted code in the p5.js repo.

Below is pseudo code for the least hacky way to do this using NodeGit that I can come up with at the spot:

index = repo.updateIndex()
origTree = index.writeTree()
index.addAll()
allTree = index.writeTree()
runGrunt()
index.addAll()
afterTree = index.writeTree()
index.readTree(origTree)
if (!Tree.equals(allTree, afterTree)) {
  console.error('good error message')
  process.exit(1)
}

The idea is to save the original commit index and then see if running grunt changes any files. Before exiting it restores the saved index. It would even be possible to restore the tree to before the command was run, if that's wanted.

[limzykenneth]

So does that mean if someone did run the grunt build step and src/data/en.yml was changed by the build step but they did not commit the tracking file, the commit will still fail?

[brysonian]

I think this can be done with husky and lint-staged, but i'll have to experiment. Is this the only aspect of the project that requires NodeGit? I' stuck because of #235 but if this feature is the only thing using NodeGit maybe we can remove it?

[lmccart]

@brysonian I believe you're correct this is the only feature that's actually using nodegit.. thanks for looking into this!

[brysonian]

Here is how husky and lint-staged could solve this particular issue (i can't test myself because of the nodegit trouble)...

Add husky and lint-staged
npm install --save-dev husky lint-staged

Add lint-staged to the precommit hook that husky adds

  "scripts": {
    "assemble": "grunt assemble",
    "build": "grunt build",
    "test": "grunt build",
    "watch": "grunt server",
    "precommit": "lint-staged"
  },

Add the lint-staged config to package.json (at the top level, sibling of scripts)

  "lint-staged": {
    "src/data/en.json": [
      "npm run build",
      "git add i18n-tracking.yml"
    ]
  },

Now the build task will be run on commit and the i18n-tracking.yml automatically added to the commit if it changed.

Of course one could also add linting etc to the hook to make sure the linting rules are obeyed.

I haven't made a PR though because i can't test locally because of #235. It seems that refactoring out nodegit is a bigger task than I anticipated. I do have some time if y'all think it is worth it, but don't totally have my head around the intended process and use cases and don't want to blunder into anything.

[lmccart]

thanks @brysonian. I believe this is the (only) place nodegit is used: https://github.com/processing/p5.js-website/blob/master/i18n.js

the git commands we're using seem to be fairly straightforward, I wonder if it would make sense to move to git-js? sounds like progress on nodegit updates for node10 may be stalled.

if you want to submit a PR without testing, I can pull in the changes and test locally to see if we can at least get that part set.

[limzykenneth]

@brysonian As @lmccart mentioned, https://github.com/processing/p5.js-website/blob/master/i18n.js is the only place nodegit is used and if git-js can be substituted for it it should be fine.

The requirements for this system is as follows:

1. If nothing changes in src/data/en.yml, i18n-tracking.yml does not change.
2. If src/data/en.yml is changed (individual lines modified or new lines added), i18n-tracking.yml should record the entry changed and the line number under the relevant file name and under all languages.
3. If there are changes to i18n-tracking.yml that are not a cause if running the build step, running the build step should not overwrite the existing changes but rather add on to it.

Whether the changes should be staged automatically or the commit should just fail I don't have too much of an opinion here.

I'm currently busy with my own work and also focusing more on the reference rendering rewrite so I can't help out too much directly but I'm happy to review the PR when you are done with it.

[brysonian]

Sorry I should have been more specific. Anyway, I see the workflow and that all makes sense. My only question is about step 3: "If there are changes to i18n-tracking.yml that are not a cause of running the build step, running the build step should not overwrite the existing changes but rather add on to it." How do we know which translations have been dealt with? Or is the idea that the translator edits that file with their commit to indicate that it's been handled?

[limzykenneth]

@brysonian Yes, that's exactly it, the translator checks off on the individual entries as they work manually and commit the changes. That's why there would be changes that are not a cause of running the build step. There's no way of determining if a translation has been checked, such as if it is just to fix typos in the English version for example, the translation obviously would not need to change.