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

new tool: git hooks (pre-push, pre-commit) #160

Closed
joyeecheung opened this issue Jan 27, 2018 · 6 comments
Closed

new tool: git hooks (pre-push, pre-commit) #160

joyeecheung opened this issue Jan 27, 2018 · 6 comments
Assignees
Labels
feature request New features for node-core-utils new tool New tool that could be added to node-core-utils stale

Comments

@joyeecheung
Copy link
Member

  1. Should be a symlink in .git/hooks/pre-push
  2. Use the remote and branch config that we have, stop users from pushing invalid commits, especially merge commits.
@joyeecheung joyeecheung added new tool New tool that could be added to node-core-utils feature request New features for node-core-utils labels Jan 27, 2018
@priyank-p
Copy link
Contributor

I worked on creating pre-push hook before, we can surely implement it. Alternatively we can also have pre-commit that runs linter only if the user wants.

@alopezsanchez
Copy link
Contributor

Questions:

  • This hook will be only for node-core-utils project, it will be a tool for collaborators of all Node.js projects, or a tool for any contributor?
  • Language of the script? Bash, Node...?

I can work of it if you don't have any inconveniences.

@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 11, 2018

This hook will be only for node-core-utils project, it will be a tool for collaborators of all Node.js projects, or a tool for any contributor?

For node core, I think. It's just a script that people can symlink to, we can ask the users if they want to symlink that during the npm post-install hook. People can also symlink to other repos if they want.

Language of the script? Bash, Node...?

Well I guess it'll need to read the .ncu/config at some point, which is JSON (also might need to read the global ~/.ncurc), so it'll be easier to write that in Node

@priyank-p
Copy link
Contributor

Language of the script? Bash, Node...?

Yep, agreed that it should be in node. But the symlinked script must be in bash, since windows also supports bash in git hooks.

And furthermore in the .git/hooks folder you need to make sure that you remove the .sample file for the hook since sometimes this causes the hook to not run.

Lastly, the hook should output info about using --no-verify flag to skip this hook.

@priyank-p priyank-p changed the title new tool: pre-push git hook new tool: git hooks (pre-push, pre-commit) Mar 11, 2018
@priyank-p priyank-p self-assigned this Mar 11, 2018
@priyank-p
Copy link
Contributor

Should this be its own ncu-git-hooks or be part of git node git node hooks? (This will be used to install/uninstall hooks)

What should it run make -j4 test and make lint for pre-commit and core-validate-commit for pre-push?

(I plan to work on this, After some issues, labeled bugs are fixed).

joyeecheung referenced this issue in nodejs/node Aug 21, 2018
`pad` is now using `toString(10)`, actually we don't need to do this. As for https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toString, `toString(N)` is a radix converter, which isn't proper here for time conversion.

PR-URL: #21906
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Ruby <rubys@intertwingly.net>
@github-actions
Copy link
Contributor

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New features for node-core-utils new tool New tool that could be added to node-core-utils stale
Projects
None yet
Development

No branches or pull requests

3 participants