Skip to content

Conversation

@dudekaa
Copy link
Contributor

@dudekaa dudekaa commented Oct 30, 2019

Hi

thanks you for making this tool happen in first place.

Back to PR, in our "development setup", we use something like "ticket number in header first" policy, mainly because of other tooling in place, etc. So I came up with this adjustment which allows everyone to use this too. It allows you to define position of resulting ticket number, plus when someone needs some custom ticket number suffix, this can be done too. Please see tests for details.

Copy link
Member

@leonardoanalista leonardoanalista left a comment

Choose a reason for hiding this comment

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

thanks for the PR, it is looking good.

* **appendBranchNameToCommitMessage**: If you use `cz-customizable` with `cz-customizable-ghooks`, you can get the branch name automatically appended to the commit message. This is done by a commit hook on `cz-customizable-ghooks`. This option has been added on `cz-customizable-ghooks`, v1.3.0. Default value is `true`.
* **ticketNumberPrefix**: {string, default 'ISSUES CLOSED:'}: Set custom prefix for footer ticker number.
* **ticketNumberSuffix**: {string, default ' '}: Set custom prefix for footer ticker number.
* **ticketNumberPosition**: {string <small>[ 'first', 'standard' or 'last' ]</small>, default none}: Set custom placement for footer ticker number.
Copy link
Member

Choose a reason for hiding this comment

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

what is the difference between none and standard. Maybe default could be standard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"standard" is default value here, none here means undefined. I saw this wording on lines 124, 125 of this file. And got me confused at first too...

ticketNumberPosition: 'last',
};

expect(buildCommit(answersTicketNumberSuffix, options)).toEqual('feat(app): this is a new feature12345');
Copy link
Member

Choose a reason for hiding this comment

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

does ticket number need space here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I fully understand you here. Space character is default for ticketNumberSuffix, so it will be there. But it doesn't make really sense, thus implementing ticketNumberSuffix option in first place, and in this test case its set to '' (empty string) on line 151, as I find it "most common use case" for setting ticketNumberPosition to "last"

Copy link
Member

Choose a reason for hiding this comment

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

I m referring to the result 'feat(app): this is a new feature12345'.
Do you intent to have 'feat(app): this is a new feature12345' or 'feat(app): this is a new feature 12345'? Note space before 12345

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I let this to ticketNumberPrefix option we already have but it will be nice to show it here. That's right.

Copy link
Member

@leonardoanalista leonardoanalista left a comment

Choose a reason for hiding this comment

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

Thanks for the PT @dudekaa ! It looks good to me.

@dmwelch
Copy link
Contributor

dmwelch commented Aug 26, 2020

@dudekaa @leonardoanalista can we get this PR "across the line" please? I would really like to see this feature! 😄

@YvonneYu
Copy link

YvonneYu commented Nov 5, 2020

We like to see ticketNumberSuffix feature too, it also need in our team's policy 👍

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.

4 participants