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

Allow emoji to be optional #35

Closed
wants to merge 3 commits into from

Conversation

steven-sheehy
Copy link
Contributor

@steven-sheehy steven-sheehy commented Feb 29, 2020

Emoji icon in release sections would be nice if it was optional in case people wanted a cleaner look and feel for their release notes (like myself).

I've ran ./mvnw verify and previously signed the CLA

Signed-off-by: Steven Sheehy <[email protected]>
Copy link
Collaborator

@snicoll snicoll 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. I don't have a strong argument for making the emoji optional and perhaps we should review this altogether.

Rather than having title + emoji and then building the title in the toString we could have a single argument that represents the title and build it upfront. When doing so, we should document that the emoji of a Section is optional rather than silently ignoring it later as you suggest here.

We'd need also to upgrade the tests to cover this scenario.

If you agree, would you be willing to review the PR in that direction?

@steven-sheehy
Copy link
Contributor Author

Just so I understand, you're advocating removal of the emoji config option and consolidating it into a single title that the user provides, correct? The default section title output would stay the same.

I can do this, I just didn't want to break Spring's tooling or a third party (like release-notes-generator-action) that depends upon the current configuration format and has customized the emoji or title.

When doing so, we should document that the emoji of a Section is optional rather than silently ignoring it later as you suggest here.

With the approach you recommend, it's more appropriate to say that the title accepts, but doesn't require, an emoji in markdown format anywhere in the string, right?

@snicoll
Copy link
Collaborator

snicoll commented Mar 10, 2020

Just so I understand, you're advocating removal of the emoji config option and consolidating it into a single title that the user provides, correct?

Actually I wasn't but that's a good idea. It would give you the flexibility to format titles any way you like. I am not aware of us using the emoji + title configuration option. Our usage is based on the ReleaseNotesSections that define default sections statically.

it's more appropriate to say that the title accepts, but doesn't require, an emoji in markdown format anywhere in the string, right?

Good idea. We could generalize by saying that the title is in markdown format and that emoji are supported.

Signed-off-by: Steven Sheehy <[email protected]>
@steven-sheehy
Copy link
Contributor Author

Thanks, I've modified the PR to remove emoji in favor of title containing all necessary information.

@steven-sheehy steven-sheehy requested a review from snicoll March 10, 2020 15:35
Signed-off-by: Steven Sheehy <[email protected]>
@steven-sheehy
Copy link
Contributor Author

Any update on this?

philwebb pushed a commit that referenced this pull request Sep 24, 2020
Fold the "emoji" property into the "title" so that it's easier to
omit if a clean looking changelog is preferred.

See gh-35
@philwebb philwebb closed this in bd12b1c Sep 24, 2020
@philwebb
Copy link
Collaborator

Thanks for the PR @steven-sheehy. This is now merged into master.

@philwebb philwebb added this to the 0.0.3 milestone Sep 24, 2020
@steven-sheehy steven-sheehy deleted the emoji-optional branch September 24, 2020 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants