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

Update release notes parsing #502

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

spencertransier
Copy link
Contributor

@spencertransier spencertransier commented Jun 23, 2023

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:

# Release Notes

## Importance Indicators
- Use `[+++]` to call out major new features.
- Use `[++]` to describe other interesting improvements
- Use `[+]` to describe minor improvements or bugfixes.

## Platforms
- Please add `iOS`, `Mac`, or `Both` to indicate which platform the change affects
- Example:
- [+++] Mac: Added a new photo picker

This remains backwards-compatible with the existing client release note formatting.

Checklist before requesting a review

  • Run bundle exec rubocop to test for code style violations and recommendations
  • Add Unit Tests (aka specs/*_spec.rb) if applicable
  • Run bundle exec rspec to run the whole test suite and ensure all your tests pass
  • Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
  • If applicable, add an entry in the 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.

@spencertransier spencertransier added the enhancement New feature or request label Jun 23, 2023
@spencertransier spencertransier requested review from mokagio and a team June 23, 2023 01:57
@spencertransier spencertransier marked this pull request as ready for review June 23, 2023 02:02
Comment on lines +15 to +19
!l.start_with?('***') &&
!l.start_with?('//') &&
!l.start_with?('#') &&
!l.start_with?('- ') &&
!l.chomp.empty?
Copy link
Contributor Author

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?

Copy link
Contributor

@AliSoftware AliSoftware Jun 23, 2023

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? }

Comment on lines -45 to -51
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

Copy link
Contributor Author

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.

Copy link
Contributor

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)

Copy link
Contributor

@AliSoftware AliSoftware left a 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.

Comment on lines -45 to -51
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

Copy link
Contributor

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)

Comment on lines +25 to +32
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
Copy link
Contributor

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 ***.

Copy link
Contributor Author

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.

@crazytonyli
Copy link
Contributor

@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.

@spencertransier
Copy link
Contributor Author

@crazytonyli Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants