-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add support for gitlab and [Unreleased] tags in git diff links #37
base: master
Are you sure you want to change the base?
Conversation
Change last line for a Unreleased tag Add output for test same Replace the last line instead of adding one Same WIP: pop" WIP: pop" WIP: pop" WIP: pop" Add else clause when there is no tag Replaces the old logic, and makes it compatible with both Replaces string with the variable for the [Unreleased] tag Reformat code Add test for CHANGELOG with [Unreleased] diffs Remove line at the end of CHANGELOG_MOCK_UNRELEASED Try something new Change git_tag with section_identifier test test2 test3 test4 testX Test y Replace the read file for tests with [Unreleased] tag
end").runner.execute(:test) | ||
|
||
modified_file = File.read(changelog_mock_unreleased_path_hook) | ||
expect(modified_file.lines.last).to eq("[Unreleased]: https://github.com/olivierlacan/keep-a-changelog/compare/v12.34.56...HEAD\n") |
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.
Ideally, could you also add verification that the before-last line has been stamped properly, e.g.:
expect(modified_file.lines.before_last).to eq("[12.34.56]: https://github.com/olivierlacan/keep-a-changelog/compare/v0.3.0...v12.34.56\n")
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.
The before_last
don't work. I really don't know how to write this without making ton of code...
@@ -57,37 +58,77 @@ def self.stamp(changelog_path, section_identifier, stamp_date, git_tag, placehol | |||
end | |||
|
|||
# Output read line | |||
file_content.concat(line) | |||
file_content.concat(last_line) |
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.
The fist time this loop iterates through it will concatenate an empty string (which results in modification of the final file) - is it intentional behaviour?
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.
It was not, I changed the logic so that it adds the current line until the Identifier is modified. It will skip that line, since it was already added, and then delay the insertion, to keep the last line for the next part of code...
It's not elegant, but the .pop()
dit not work on the array...
if last_line.include? 'https://github.com' # GitHub uses compare/olderTag...newerTag structure | ||
previous_previous_tag = %r{(?<=compare\/)(.*)?(?=\.{3})}.match(last_line) | ||
previous_tag = /(?<=\.{3})(.*)?/.match(last_line) | ||
if last_line.include? 'https://github.com' or last_line.include? 'http://gitlab.' or last_line.include? 'https://gitlab.com' # GitHub and Gitlab use compare/olderTag...newerTag structure |
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.
Would it make sense to use http(s)?://gitlab.com
regex?
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 really don't know how to do that in Ruby... I worked out how to to the rest, this is my first time in this language.
previous_previous_tag = %r{(?<=compare\/)(.*)?(?=\.{3})}.match(last_line) | ||
previous_tag = /(?<=\.{3})(.*)?/.match(last_line) | ||
if last_line.include? 'https://github.com' or last_line.include? 'http://gitlab.' or last_line.include? 'https://gitlab.com' # GitHub and Gitlab use compare/olderTag...newerTag structure | ||
git = 'github/gitlab' |
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.
#Did you consider declaring this string as constant at the top? Since it's used later on for comparison, it's kinda error prone.
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.
Great idea, this is done ;)
Thanks a lot for your contribution @Eradash . I have a few comments to make your PR awesome and we can release it right after, looking forward for your feedback |
Restore original indent Place the condition change at the correct place I'm guessing this may start causing us problems either soon or never.
I just finished the modifications, and commented those I could not do |
Hi @Eradash is this PR in mergeable state or could you also add unit tests for gitlab as mentioned above (would prefer so)?
|
What's the status on this PR? Suggestion: Can't we use a |
This adds the support for gitlab in diff links at the bottom of CHANGELOG.md
It also adds the support of
[Unreleased]
tags, with a reference to HEAD (both for github and bitbucket), to see the diff between last tag and current HEADI added a test for the [Unreleased] tag, but not for gitlab. Will work on this in a future pull request.