-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[Tooling] Fix handling of empty release notes #21565
Conversation
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.
Approving to unblock, but left a suggestion to improve readability of implementation a bit if you want to apply it before merge.
fastlane/lanes/localization.rb
Outdated
File.write(File.join(changelog_dir, 'default.txt'), '') | ||
end | ||
# Skip downloading release notes translations since the source is empty | ||
files.delete("release_note_#{version_suffix}") |
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.
Reading this made me think at first that this was deleting a file with this name. Took me a minute to realize this was instead just removing the entry with that key from the files
Hash
instead.
Wouldn't it be more readable to do the check first, and only assign the value of files
for that key (on line 201) if not empty?
In other words, write this block of code as:
if !File.exist?(…) || File.read(…).strip.empty?
# Clear release notes if the source file is empty
# …
else
# Make sure to also download the release notes translations (given we had non-empty original copy)
files["release_note_#{version_suffix}"] = { … }
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.
Good suggestion 👍 Updated on b590c80
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
Quality Gate passedIssues Measures |
Reference: p1733408151597889-slack-CC7L49W13
The main change is to better handle in the lane
download_metadata_strings
the scenario when the source release notes files (WordPress/metadata/release_notes.txt
orWordPress/jetpack_metadata/release_notes.txt
) are empty, clearing the localized files used by Fastlane (fastlane/metadata/android/*/changelogs/default.txt
orfastlane/jetpack_metadata/android/*/changelogs/default.txt
).Additionally, I've also updated the lane
download_metadata_strings
lane to use keywords syntax for parameters.To Test:
You can run the
download_metadata_strings
lane using the command below (avoiding commiting/pushing) to make sure thedefault.txt
files are cleared when the source notesrelease_notes.txt
are empty. Whenrelease_notes.txt
has content, thedefault.txt
files should be fetched from GlotPress using the specifiedversion
.bundle exec fastlane download_metadata_strings version:25.2 skip_commit:true