fix(changelog): consider placeholder for old version #664
Conversation
|
hi @bernardcooke53 , do you know if there are test cases to cover the case where template_dir does not exist https://github.com/python-semantic-release/python-semantic-release/blob/master/semantic_release/cli/commands/changelog.py#L57? |
|
Hi @roggervalf - apologies, I've been quite unwell for the last couple of weeks and haven't really been able to look at this. I will try to look in more detail in the next couple of days, I would like to see an additional piece of logic:
I appreciate that might not be the most clear explanation, will come back to this in the near future |
thank you @bernardcooke53, no worries, take your time |
| @@ -27,7 +27,7 @@ | |||
|
|
|||
|
|
|||
| def tags_and_versions( | |||
| tags: Iterable[Tag], translator: VersionTranslator | |||
| tags: Iterable[Tag], translator: VersionTranslator, only_last_release: bool = False | |||
There was a problem hiding this comment.
I don't think it's necessary to add this, it's not the purpose of tags_and_versions at all. There's only one location where you intend to set this to True, I don't see why you can't just add the [0] there?
| f.truncate() | ||
| existed_changelog_sections = existed_changelog_text.split(CHANGELOG_PLACEHOLDER) | ||
| if len(existed_changelog_sections) > 1: | ||
| log.warning("Deprecation: Placeholder is detected, only new changes will be added.") |
There was a problem hiding this comment.
👍 for warning, but it would be good to include a link to Migrating an Existing Changelog
| f.seek(0) | ||
| existed_changelog_text = f.read() | ||
| f.truncate() |
There was a problem hiding this comment.
This is error-prone - if there's an exception later on then we will have truncated the changelog without writing any text back to it
| existed_changelog_text = f.read() | ||
| f.truncate() | ||
| existed_changelog_sections = existed_changelog_text.split(CHANGELOG_PLACEHOLDER) | ||
| if len(existed_changelog_sections) > 1: |
There was a problem hiding this comment.
I think it's a much simpler check to have f"{CHANGELOG_PLACEHOLDER}\n" in f.readlines() - avoids trying to split. The text could (by chance) by in a commit message which would lead to issues.
You can always do
existing_changelog_text = f.read()
f.seek(0)
lines = f.readlines()to get both, or even
lines = f.readlines()
existing_changelog_text = "".join(lines)| changelog_text = render_default_changelog_file(env) | ||
| with open(str(changelog_file), "w+", encoding="utf-8") as f: | ||
| f.write(changelog_text) | ||
| with open(str(changelog_file), "a+", encoding="utf-8") as f: |
There was a problem hiding this comment.
why open in append mode?
There was a problem hiding this comment.
hey @bernardcooke53, because I'm reading and writing
| existed_changelog_sections = existed_changelog_text.split(CHANGELOG_PLACEHOLDER) | ||
| if len(existed_changelog_sections) > 1: | ||
| log.warning("Deprecation: Placeholder is detected, only new changes will be added.") | ||
| rh = ReleaseHistory.from_git_history( |
There was a problem hiding this comment.
We always iterate in reverse order over the commits, which means the latest release is the first one inserted into ReleaseHistory.released.
You can get the last release by doing:
full_rh = ReleaseHistory.from_git_history(<the existing params>)
latest_release_version = next(iter(full_rh.unreleased), None)
latest_release = full_rh.released.get(latest_release_version, {})
rh = ReleaseHistory(unreleased=full_rh.unreleased, released=latest_release)This works because dicts remember their insertion order so next will find the oldest-inserted key
There was a problem hiding this comment.
with these suggestions I'm not sure how to keep the existing changelog, as I don't want to override previous changelogs, only add the latest changelog
There was a problem hiding this comment.
General comment - there's no testing for this functionality, would be nice to see some tests added for it
There was a problem hiding this comment.
I think I need some help on making a test for it
|
This PR is stale because it has not been confirmed or planned by the maintainers and had been open 60 days with no recent activity. It will be closed in 10 days, |
|
This PR was closed because activity was dormant for 70 days. |
ref #652