Improve section ordering in scripts/github-changelog.cr
#11770
Conversation
scripts/github-changelog.cr
Outdated
| @@ -163,11 +165,17 @@ sections = array.group_by { |pr| | |||
| end || "Other" | |||
| } | |||
|
|
|||
| titles = sections.keys.sort! | |||
| titles = [] of String | |||
| ["Compiler", "Language", "Standard Library", "Tools", "Other"].each do |main_section| | |||
There was a problem hiding this comment.
I guess this could be changed to ["Language", "Standard Library", "Compiler", "Tools", "Other"] because language and stdlib are more impactful to the average user than compiler changes?
| last_title1 = nil | ||
|
|
||
| titles.each do |title| | ||
| prs = sections[title] | ||
| prs = sections[title]? || next |
There was a problem hiding this comment.
| prs = sections[title]? || next | |
| next unless prs = sections[title]? |
There was a problem hiding this comment.
No. I think assignments in trailing conditionals are a code smell.
There was a problem hiding this comment.
And next in assignments is fine with you? What's the logic here?
There was a problem hiding this comment.
| prs = sections[title]? || next | |
| prs = sections[title]? | |
| next unless prs |
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Using next / return or raise within the assignments should be considered as a code smell.
That's an established idiom, similar to
x || returnandx || raiseetc. 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.
There was a problem hiding this comment.
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.
This patch establishes a fixed order for sections in the changelog generator.