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

Jump to bottom

Update release notes parsing #502

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

Update release notes parsing #502

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c9058f
Add # and - to release notes parser
spencertransier Jun 23, 2023
baac2e7
Update tests
spencertransier Jun 23, 2023
add2aac
Apply rubocop fixes
spencertransier Jun 23, 2023
7bc0a80
Update CHANGELOG.md
spencertransier Jun 23, 2023
265ffb9
Update CHANGELOG.md
spencertransier Jun 23, 2023
6d6b969
Merge branch 'trunk' into update/changelog-parsing
spencertransier Jul 4, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
### New Features

- Adds `if_exists` parameter to `upload_to_s3` action, with possible values `:skip`, `:fail`, and `:replace`. [#495]
- Adds additional formatting options for release note files. They can now include Markdown headers (`#`, `##`, etc.) and list items (Starting with `-`) at the top of the file. [#508]

### Bug Fixes

Expand Down
8 changes: 7 additions & 1 deletion lib/fastlane/plugin/wpmreleasetoolkit/helper/release_notes_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,13 @@ def self.add_new_section(path:, section_title:)

# Find the index of the first non-empty line that is also NOT a comment.
# That way we keep commment headers as the very top of the file
line_idx = lines.find_index { |l| !l.start_with?('***') && !l.start_with?('//') && !l.chomp.empty? }
line_idx = lines.find_index do |l|
!l.start_with?('***') &&
!l.start_with?('//') &&
!l.start_with?('#') &&
!l.start_with?('- ') &&
!l.chomp.empty?
Comment on lines +15 to +19
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 •
edited
Loading

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

end
# Put back the header, then the new entry, then the rest
# (note: '...' excludes the higher bound of the range, unlike '..')
new_lines = lines[0...line_idx] + ["#{section_title}\n", "-----\n", "\n", "\n"] + lines[line_idx..]
Expand Down
31 changes: 24 additions & 7 deletions spec/release_notes_helper_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,23 @@
HEADER
end

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
Comment on lines +25 to +32
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.


it 'adds a new section after any `- ` comments on top' do
run_release_notes_test <<~HEADER
- This is a line item
- And this is another line item that we want to include

HEADER
end

it 'does consider empty lines as header' do
run_release_notes_test("\n\n\n")
end
Expand All @@ -35,20 +52,20 @@
*** with both double-slash style comment lines
*** and triple-star style ones.

- List item
- Another list item

# Markdown Header
## H2 Markdown Header




// It also contains some empty lines we want to count as part of the pinned lines.

HEADER
end

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

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

it 'does not consider ** as comments' do
prefix = <<~NOT_HEADER
** This is not a comment
Expand Down