-
Notifications
You must be signed in to change notification settings - Fork 9
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
Update release notes parsing #502
base: trunk
Are you sure you want to change the base?
Conversation
!l.start_with?('***') && | ||
!l.start_with?('//') && | ||
!l.start_with?('#') && | ||
!l.start_with?('- ') && | ||
!l.chomp.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubocop says this formatting is fine, but it seems strange to have the last four lines indented more.
I broke them into separate lines for readability. Is there a better way of doing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the additional indentation is because the subsequent lines are a continuation of the first line so I agree it can look strange, but it's expected at least.
Another option would be to extract this condition in a function to make this line shorter and use an array of comment prefixes. Something like:
def comment?(line)
['***', '//', '# ', '- '].any? { |s| line.start_with?(s) }
end
Then we could go back to write this code as a one-liner:
line_idx = lines.find_index { |l| !comment?(l) && !l.chomp.empty? }
it 'does not consider # as comments' do | ||
prefix = <<~NOT_HEADER | ||
# This is not a comment | ||
NOT_HEADER | ||
run_release_notes_test('', prefix + FAKE_CONTENT) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test isn't needed anymore now that #
's can be used in the header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be worth it to make sure that any line starting with #
after the header would not be skipped though, like if we have #
in the middle of a release note content (just like we can have -
in the middle of existing release notes and those should not be considered as comments — only the lines at the top, before the first valid header, should be ignored)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I agree with allowing lines starting with #
to be considered comments, because many of our CHANGELOGs in various repos use the ## 1.2.3
format for version section headers (as opposed to 1.2.3\n---
like we use in the unit tests)
I've provided some ideas for alternatives in comments below instead.
it 'does not consider # as comments' do | ||
prefix = <<~NOT_HEADER | ||
# This is not a comment | ||
NOT_HEADER | ||
run_release_notes_test('', prefix + FAKE_CONTENT) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be worth it to make sure that any line starting with #
after the header would not be skipped though, like if we have #
in the middle of a release note content (just like we can have -
in the middle of existing release notes and those should not be considered as comments — only the lines at the top, before the first valid header, should be ignored)
it 'adds a new section after any `#` comments on top' do | ||
run_release_notes_test <<~HEADER | ||
# This is a Markdown header | ||
## This is another kind of Markdown header | ||
### This is an H3 Markdown header | ||
|
||
HEADER | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this might not be what we want, because while the FAKE_CONTENT
from this test suite suggests that all our CHANGELOG files use ---
to underline section titles, many of our other changelogs (including release-toolkit's own CHANGELOG) use the ## 1.2.3
markdown syntax instead for section titles.
I'm not fully sure that our current release_notes_helper
implementation supports using the ## x.y.z
format instead of x.y.z\n---
to be honest, maybe it doesn't. But I could see a future where we would prefer adopting that more common ## x.y.z
markdown syntax for headers, and in such case if we allow here those to be considered comment headers, this would prevent us from allowing them to be valid ## 1.2.3
version sections in the CHANGELOG.
For that reason, I'm not sure I agree for us to consider #
, ##
and ###
sections as being "header comments" in our CHANGELOGs.
An alternative that we could consider would be to use HTML comments:
- Either to use them as header comments, i.e. you can start your
CHANGELOG.md
file with<!--
, put some instructions, then-->
, before starting the rest of the CHANGELOG content, and the helper would have to insert a new section for a new version only after that<!-- … -->
part - Or make a convention that one can use
<!-- CHANGELOG START -->
or<!-- New Version Marker -->
or something in the CHANGELOG to mark where the helper is supposed to insert new sections in the CHANGELOG, making it even more flexible by allowing anything before that section (even# Instructions
setions and such)
I think that solution shouldn't be too hard to implement, something like:
line_idx = if lines.first.start_with('<!--')
lines.find_index { |l| l.end_with('-->') } &+ 1
else
lines.find_index { |l| l.chomp == '<!-- INSTRUCTION END MARKER -->' } &+ 1
end
line_idx ||= lines.finx_index { |l| !l.start_with('***') && !l.start_with('//') && !l.chomp.empty? }
That way, if the file starts with <!--
then we will start inserting on the line after the closing -->
, otherwise we'll try to find the special line with content <!-- INSTRUCTION END MARKER -->
if it exist (and if so, start inserting after it), and finally if none of those found anything, we fall back to the current behavior of inserting after any first lines starting with //
or ***
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative that we could consider would be to use HTML comments:
I really like this approach! Seems much simpler and easy to just have anything above a <!-- CHANGELOG START -->
be considered the file header and to insert the new section after that.
@spencertransier The two new "build-and-test" required PR checks are added from #492. Merging the trunk branch into this PR should satisify those two new required checks. |
@crazytonyli Thank you! |
What does it do?
This PR adds more parsing options to the
release_notes_helper.rb
so that Markdown headers and lists can be used at the top of the release notes/changelog files.@mokagio suggested this change so that a client could use this kind of formatting in their release notes file:
This remains backwards-compatible with the existing client release note formatting.
Checklist before requesting a review
bundle exec rubocop
to test for code style violations and recommendationsspecs/*_spec.rb
) if applicablebundle exec rspec
to run the whole test suite and ensure all your tests passCHANGELOG.md
file to describe your changes under the appropriate existing###
subsection of the existing## Trunk
section.MIGRATION.md
file to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.