Commitplease #1523

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Owner

jzaefferer commented Feb 21, 2014

Two commits: First sorting the devDependencies in package.json, second adding commitplease, which installs as a commit-msg hook to check commit messages as you commit. As y'all should already know.

Member

mgol commented Feb 23, 2014

@jzaefferer Why is this on the branch in the jquery repo instead your own?

Owner

jzaefferer commented Feb 23, 2014

No particular reason. Just my default work flow for repos where I have write access. Makes it easy for collaborators to take over something I started.

Member

mgol commented Feb 23, 2014

It's just sth that we've never practiced in Core except for very specific branches on which there is an ongoing work we all want to follow (like AMD), usually long-living branches. Creating short-lived branches on the upstream repo creates a noise when listing branches which is especially visible when using some Git GUI like SourceTree, it triggers IRC notifications etc. And if everyone started doing that, there would be a lot of clutter in the repo.

I'd prefer to not create branches on upstream in such cases.

Owner

jzaefferer commented Feb 23, 2014

Okay, will keep that in mind and use a branch on my fork the next time.

That said, anything holding this up from merging?

Member

mgol commented Feb 23, 2014

LGTM.

Owner

jzaefferer commented Feb 25, 2014

@dmethvin says he still can't install on Windows, hoping for some more details from him.

Owner

dmethvin commented Feb 25, 2014

This turned out to be the permissions problem. It's going to be a real pain to need to use admin to install, and we would need to update the readme to say that. Is there a reason this has to use a link, as opposed to just copying a file?

Owner

dmethvin commented Mar 1, 2014

Is copying the file an option?

Owner

dmethvin commented Mar 16, 2014

I would like to land this but prefer to have an approach that doesn't require Windows users to run as administrator. Is copying the file an option, @jzaefferer ?

Owner

jzaefferer commented Mar 16, 2014

I will look into this tomorrow. Uninstalling is probably a little more complicated, but that's the only decent drawback I can think of.

Owner

jzaefferer commented Mar 16, 2014

Currently the script only removes a symlink that it created, instead of just deleting whatever hook is there. I would like to have a equivalent check for the copied file, to avoid deleting a custom hook.

Owner

scottgonzalez commented Mar 17, 2014

Just stick a comment at the top of the file with something unique, like /*commitplease*/, and check for that before deleting.

@jzaefferer jzaefferer added a commit that referenced this pull request Mar 20, 2014

@jzaefferer @dmethvin jzaefferer + dmethvin Build: Add commitplease for commit msg checking
Closes gh-1523
(cherry picked from commit 176a2913260b4ec890118137ccc9a290064a59be)

Conflicts:
	package.json
fe8904c
Owner

dmethvin commented Mar 20, 2014

Just a FYI, if you are running on Windows and installed with admin, you may need to log in as admin and delete the node_modules subdir so that npm install will work okay as non-admin.

dmethvin deleted the commitplease branch Mar 21, 2014

Owner

jzaefferer commented Apr 1, 2014

@dmethvin since you merged this, does your last comment mean there's nothing to fix in commitplease?

Owner

dmethvin commented Apr 2, 2014

Nobody seems to run Windows but me so I might as well land it until things were fixed. We'll just need to bump the version.

Member

mgol commented Apr 2, 2014

Our not-in-the-team contributors can potentially be on Windows.

Michał Gołębiowski

jzaefferer referenced this pull request in jzaefferer/commitplease Apr 8, 2014

Closed

Support Windows #8

@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014

@jzaefferer @dmethvin jzaefferer + dmethvin Build: Add commitplease for commit msg checking
Closes gh-1523
(cherry picked from commit 176a2913260b4ec890118137ccc9a290064a59be)

Conflicts:
	package.json
8f0d926
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment