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

Validate: Add subjectWhitelist option, excepting specific subjects #29

Merged
merged 1 commit into from
Oct 22, 2014

Conversation

jzaefferer
Copy link
Owner

See the readme updats for usage details.

Fixes #26
Fixes #28

@scottgonzalez @mislav would be great if you could take a look at this

@scottgonzalez
Copy link
Contributor

This seems ok, but this isn't really a whitelist, it's a required pattern.

@jzaefferer
Copy link
Owner Author

requiredPattern doesn't sound like an improvement. Could also be described as acceptable exceptions, but that doesn't make a good identifier either.

@@ -6,7 +6,8 @@ var merge = require( "mout/object/merge" ),
limits: {
subject: 72,
other: 80
}
},
subjectWhitelist: /^(fixup|squash)!|^\[[^\]]+\]:/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess configurability is useful, but if you as a project maintainer decide to configure this value and forget to explicitly bring in the fixup|squash bit, you would be inadvertently breaking these git features again for other committers. Since there is really no reason why would you ever want to de-whitelist these prefixes, I suggest that they're hardcoded somewhere instead of being configurable. The square bracket syntax could still be configurable and turned off at will.

Both fixup! and squash! are git-specific. Any component in square brackets
like [Tmp] or [fix] is also considered valid, to be manually squashed later.

Fixes #26
Fixes #28
@jzaefferer
Copy link
Owner Author

Thank you both for the feedback. I've now changed the approach, removing the option and instead hardcoding the accepted patterns. I can still make it configurable later, if the needs ever arises. For now I'm avoiding the naming and some other issues (can't put regex in JSON).

What do you think?

@mislav
Copy link

mislav commented Oct 21, 2014

Yep, :shipit:

@jzaefferer jzaefferer merged commit 39475c8 into master Oct 22, 2014
@jzaefferer
Copy link
Owner Author

I've published this as part of 2.0.0 to npm.

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.

Whitelist component for fixup commits Allow git commit --fixup and --squash features
3 participants