-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: Add signoff option #120
feat: Add signoff option #120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks so much for the PR! AND documentation and tests! 🙌🎉
I left a couple questions inline - Also, do you think the argument would be better named —signoff
, to match the git argument that people might be looking for?
@@ -1475,6 +1475,45 @@ describe('cli', function () { | |||
expect(gitArgs).toHaveLength(0); | |||
}); | |||
|
|||
|
|||
it('--signedoff adds signed-off-by to the commit message', async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s late here so maybe I’m missing something, but I can’t see how this test confirms what it says in the it statement- is it supposed to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied the previous test that was testing for the --sign
option and how it affected the git commit
command;
Just to confirm - admit I've not dug into deep details of how the tests are structured - I've done the 'inverse' test and deliberately broke the code and the test flags this up...
● cli › with mocked git › --signedoff adds signed-off-by to the commit message
expect(received).toEqual(expected) // deep equality
- Expected - 1
+ Received + 1
@@ -1,8 +1,8 @@
Array [
"commit",
- "--signoff",
+ "--wibble",
"CHANGELOG.md",
"package.json",
"package-lock.json",
"-m",
"chore(release): 1.0.1",
Gives me some confidence that test would act as a regression check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, yep, makes sense. I’m not the original author, so I’m not across exactly how all the tests work
|
thanks :-) I'd debated with myself on that exact question. Using Would Happy to go within either spelling :-) |
c1bd130
to
6bc64c2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #120 +/- ##
==========================================
+ Coverage 94.42% 94.43% +0.01%
==========================================
Files 25 25
Lines 466 467 +1
==========================================
+ Hits 440 441 +1
Misses 26 26 ☔ View full report in Codecov by Sentry. |
I see what you are saying about the ambiguity, but I reckon that’s a risk either way. Even if the alternative is more ambiguous, I reckon that the consistency with git is worth it. |
a577ff7
to
a1609a7
Compare
Signed-off-by: mbwhite <whitemat@uk.ibm.com>
a1609a7
to
42b05bc
Compare
Thanks @TimothyJones |
Thank you for the contribution! Releasing 12.1.0 now, should be live shortly. |
Our current process required that the
git commit --signoff
is used to add asigned-off-by
trailer to the commit message.Adding an additional CLI option to permit this to be added.