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

Add contributors on own line option #1004

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

boardfish
Copy link

@boardfish boardfish commented Jan 5, 2022

Hey, folks! I'm investigating adopting this for ViewComponent. As I identified here, we'd need a few tweaks to do so and reflect the current changelog. This is one of them - my implementation feels a little bit sketchy so far, so I'd appreciate any advice I can get on how to comfortably have this sit alongside the current code.

At a glance, it'll take PRs from looking like this:

- Add contributors on own line options [\\#1004](https://github.com/github-changelog-generator/github-changelog-generator/pull/1004) ([@boardfish](github.com/boardfish)

to this:

- Add contributors on own line options [\\#1004](https://github.com/github-changelog-generator/github-changelog-generator/pull/1004)

  *[@boardfish](github.com/boardfish)*

Copy link

@yykamei yykamei left a comment

Choose a reason for hiding this comment

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

Hi, I'm not a maintainer, but I'm interested in this pull request. In my opinion, It looks good 👍 but I left a trivial comment.

I wonder what's next if it's ready to merge 👀

@@ -97,14 +98,18 @@ def body_till_first_break(body)
def issue_line_with_user(line, issue)
return line if !@options[:author] || issue["pull_request"].nil?

user = issue["user"]
user = user(issue["user"])
return "#{line} ({Null user})" unless user
Copy link

Choose a reason for hiding this comment

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

GitHubChangelogGenerator::Section#user seems to only return String as far as I see, so I think this line is no longer needed.

Suggested change
return "#{line} ({Null user})" unless user

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.

None yet

2 participants