Skip to content

Husky i18n tracking #240

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

Merged
merged 4 commits into from
Jun 30, 2018
Merged

Husky i18n tracking #240

merged 4 commits into from
Jun 30, 2018

Conversation

brysonian
Copy link
Contributor

@brysonian brysonian commented Jun 27, 2018

This refactors the i18n test to use simple-git also adds the test via a precommit hook. The hook doesn't build the entire site, it just runs the test for translation changes, that's easy to change, but I opted for this because it is faster to run vs. executing an entire build.

closes #235
closes #234
Possibly fixes #237

@Zalastax
Copy link
Member

My opinion is that it is better to use NodeGit so that we can guarantee consistency. simple-git uses the git executable that the user has installed and we will thereby be exposed by potential differences between git versions. I know that NodeGit is inconvenient at first, but I think it's worth it.

@brysonian
Copy link
Contributor Author

Yes for sure, but it is more than inconvenience, it actively prevents users from contributing to the project; some folks can't downgrade to a different node version. It also seems unlikely that git is going to change the format of diffs, which would break a tremendous number of projects.

@limzykenneth limzykenneth mentioned this pull request Jun 27, 2018
@limzykenneth
Copy link
Member

I think it's ok to switch to simple-git as the format of diffs as @brysonian mentioned are not very likely to change between versions and this is just piggybacking on git diff to find the line number and entry name in a yaml file. We also won't want to force a maximum version of node.js that a contributor can use (minimum versions are fine but not maximum version as nodegit would require us to).

I'll do a more thorough test of the PR in a couple days, still a bit tied up at the moment. 😃

@lmccart
Copy link
Member

lmccart commented Jun 27, 2018

thanks @brysonian this is super helpful! @limzykenneth if you can take care of testing and merging that would be great. but let me know if you need help and I can take this on instead.

@limzykenneth
Copy link
Member

When I try to commit after changing en.yml I got this error:

husky > npm run -s precommit (node v8.11.1)

 ❯ Running tasks for src/data/en.yml
   ✖ grunt i18n --changed 
     git add i18n-tracking.yml
✖ "grunt i18n --changed " found some errors. Please fix them and try committing again.

module.js:549
throw err;
^

Error: Cannot find module '../lib/cli'
at Function.Module._resolveFilename (module.js:547:15)
at Function.Module._load (module.js:474:25)
at Module.require (module.js:596:17)
at require (internal/module.js:11:18)
at Object.<anonymous> (/Users/kennethlim/Sites/p5.js-website/node_modules/.bin/grunt:12:15)
at Module._compile (module.js:652:30)
at Object.Module._extensions..js (module.js:663:10)
at Module.load (module.js:565:32)
at tryModuleLoad (module.js:505:12)
at Function.Module._load (module.js:497:3)

husky > pre-commit hook failed (add --no-verify to bypass)

Simply running grunt, grunt i18n and grunt i18n --changed all passed without problem. The node_modules folder is fresh (deleted then npm install). Let me know if you need more info.

@brysonian
Copy link
Contributor Author

Hrm, sounds like an NPM issue. Since you are using a fresh node_modules, made try clearing the npm cache npm cache clean --force and npm i again? Just to be sure it wasn't a node version thing I spun up a new vagrant ubuntu box and installed node 8.11.3 and the site built as expected. Can anyone else test this?

@limzykenneth
Copy link
Member

Somehow that worked, not sure if it's cleaning the cache or maybe it's to do with the lock file. I'll do the rest of my test and come back to you. Thanks.

@limzykenneth
Copy link
Member

Everything looks to be checking out as intended, I'll resolve the conflicts and merge this then.

@limzykenneth limzykenneth merged commit 5a2c492 into master Jun 30, 2018
@limzykenneth limzykenneth deleted the husky-i18n-tracking branch June 30, 2018 03:38
@lmccart
Copy link
Member

lmccart commented Jul 16, 2018

I'm seeing this error when I try to run git commit. any ideas?

lmccart:p5.js-website lmccart$ git commit -m 'updating i18n tracking file'
husky > npm run -s precommit (node v6.9.5)

 ❯ Running tasks for src/data/en.yml
   ✖ grunt i18n --changed 
     git add i18n-tracking.yml
✖ "grunt i18n --changed " found some errors. Please fix them and try committing again.
>> Local Npm module "assemble" not found. Is it installed?
Warning: Task "/Users/lmccart/Documents/p5/p5.js-website/src/data/en.yml" not found. Use --force to continue.

Aborted due to warnings.


@limzykenneth
Copy link
Member

@lmccart Have you tried reinstalling node_modules? If so maybe tried the steps @brysonian mentioned, clean out the lockfile as well if it still doesn't work.

@brysonian
Copy link
Contributor Author

brysonian commented Jul 16, 2018 via email

@lmccart
Copy link
Member

lmccart commented Jul 16, 2018

ugh sorryyy I should've tried all the things in this thread first. 😳will report back.

@lmccart
Copy link
Member

lmccart commented Jul 16, 2018

yep that worked fine now. thanks for the quick help.

@limzykenneth
Copy link
Member

@lmccart How did you solve this issue? I am encountering it and nothing seems to fix it. From what I found online, it seems we need to depend on a different module to be able to use it as a grunt plugin https://stackoverflow.com/questions/34692592/grunt-assemble-module-not-found

@limzykenneth
Copy link
Member

limzykenneth commented Jul 23, 2018

It does seem to be the case where after updating from 0.4.42 to 0.24.3 at processing/p5.js-website@eeeefc8 would have caused this issue. I'll try to switch to using grunt-assemble, hopefully it just works.

Edit: That worked, now merged as part of #243, everything should work the same, just added the grunt-assemble package and load that in the Gruntfile.js instead of assemble directly.

@lmccart
Copy link
Member

lmccart commented Jul 23, 2018

Whoops sorry about that, thanks for updating it.

shibomb added a commit to shibomb/p5.js-website-legacy-20240504 that referenced this pull request Jul 16, 2023
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.

Site fails to build Nodegit causes npm install to fail on Node 10 translation changes not getting marked
4 participants