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.

Sign up for GitHub

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

Improve section ordering in scripts/github-changelog.cr #11770

Merged
merged 3 commits into from
Mar 17, 2022
Merged

Improve section ordering in scripts/github-changelog.cr #11770

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7de8f1a
Improve section ordering in `scripts/github-changelog.cr`
straight-shoota Jan 4, 2022
5af19bf
Change ordering to place compiler section further down
straight-shoota Jan 27, 2022
c0b0485
Add sub-sections for compiler and tools
straight-shoota Jan 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions scripts/github-changelog.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,17 @@ sections = array.group_by { |pr|
when .starts_with?("topic:lang")
break "Language"
when .starts_with?("topic:compiler")
break "Compiler"
if label == "topic:compiler"
break "Compiler"
else
break "Compiler: #{label.lchop("topic:compiler:").titleize}"
end
when .starts_with?("topic:tools")
break "Tools"
if label == "topic:tools"
break "Tools"
else
break "Tools: #{label.lchop("topic:tools:").titleize}"
end
when .starts_with?("topic:stdlib")
if label == "topic:stdlib"
break "Standard Library"
Expand All @@ -163,11 +171,17 @@ sections = array.group_by { |pr|
end || "Other"
}

titles = sections.keys.sort!
titles = [] of String
["Language", "Standard Library", "Compiler", "Tools", "Other"].each do |main_section|
titles.concat sections.each_key.select(&.starts_with?(main_section)).to_a.sort!
end
sections.keys.sort!.each do |section|
titles << section unless titles.includes?(section)
end
last_title1 = nil

titles.each do |title|
prs = sections[title]
prs = sections[title]? || next
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
prs = sections[title]? || next
next unless prs = sections[title]?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. I think assignments in trailing conditionals are a code smell.

Copy link
Contributor

Choose a reason for hiding this comment

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

And next in assignments is fine with you? What's the logic here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
prs = sections[title]? || next
prs = sections[title]?
next unless prs

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think that's fine. It's handling a null state and breaks from the current iteration as a result. That's an established idiom, similar to x || return and x || raise etc. of which you can find many instances in stdlib, for example.
Separating the expressions is also fine, of course. I just don't see it's necessary.
Unless there's reasonable concern about the readability, I'd like to avoid useless style discussions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using next / return or raise within the assignments should be considered as a code smell.

That's an established idiom, similar to x || return and x || raise etc. of which you can find many instances in stdlib, for example.

No, it's not. In stdlib is used only in 5 places (not counting specs) - see https://cs.github.com/crystal-lang/crystal?q=%2F%5Cs%3D%28.%2B%3F%29+%5C%7C%5C%7C+%28next%7Creturn%7Cbreak%29%5CW%2F+-path%3A%2Fspec%2F

Unless there's reasonable concern about the readability, I'd like to avoid useless style discussions.

Useless for you perhaps, so please keep your judgements for yourself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I didn't mean to pass any judgement. Useless discussions are useless, that's a fact. Whether this particular style discussion is useless depends on the condition of whether there's reasonable argument.

Your link doesn't work for me, and I'm sure its results are incomplete. A local code search shows at least 80 instances of || plus trailing break expression in src, a quarter of them in assignments. So I'm pretty sure this can be considered established. If you'd like to change that, please start a separate discussion. But we should refrain from discussing general style issues in unrelated PRs.

title1, _, title2 = title.partition(": ")
if title2.presence
if title1 != last_title1
Expand Down