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

Fix isIncrement to return only the version requested #28

Merged
merged 1 commit into from
Aug 10, 2022
Merged

Fix isIncrement to return only the version requested #28

merged 1 commit into from
Aug 10, 2022

Conversation

k2snowman69
Copy link
Contributor

@k2snowman69 k2snowman69 commented Aug 5, 2022

Fixes #27

A few issues that made this take longer than I expected:

  1. strictLatest seems to cause a failure if the changelog only contains Unreleased. There is even a test case which makes me feel this is intended but it really didn't make any sense. I think this is related to Unclear how should the initial release look like #19 and I've marked the lines that need to be removed. I'd like to note, removing these lines however seems to completely eliminate the usage of strictLatest which I'm not sure why it exists
  2. The test harness factory from release-it/test/util/index.js doesn't actually set isIncrement if it's set as an option. isIncrement is always true no matter what. This is causing the tests to fail. It took me 4 hours to figure out this was my core issue. I spent another hour trying to figure out how to set it and then manually manipulated the variable in memory to make sure my code is good. Without fixing this in the core harness, this code will fail tests.

Some thoughts working in this project

  1. The naming of the changelogs isn't clear to what the content is.
    1. CHANGELOG-FOO - Maybe a better name is "invalid" because it doesn't match the syntax expected?
    2. MISSING - Better name would be CHANGELOG-UNRELEASE_FULL_RELEASE_MISSING
    3. EMPTY - Better name would be CHANGELOG-UNRELEASE_EMPTY_RELEASE_FULL
    4. FULL - Better name would be CHANGELOG-UNRELEASE_FULL_RELEASE_FULL
    5. CHANGELOG-NO-STRICT is the same as CHANGELOG-UNRELEASED
    6. CHANGELOG-FULL only differs from CHANGELOG-UNRELEASED by having a # Changelog prefixed
  2. Several variations of changelogs aren't being tested such as CHANGELOG-UNRELEASE_EMPTY_RELEASE_EMPTY
  3. Tests could use some organizing. Either splitting up the different options into different files or making use of something similar to jest's describe block would help.
  4. A few options are a bit confusing
    1. keepUnreleased - I thought this would "keep the unreleased title" in the changelog. However it seems to just not version anything which seems to be the same as isIncrement... so should this be deprecated/removed?
    2. addUnreleased - Isn't having an "Unreleased" section part of the https://keepachangelog.com/en/1.0.0/ requirements? So maybe should we just be verifying there is always an unreleased section?

I'm going to stop there, anyway just some thoughts for you.

let endIndex = this.changelogContent.indexOf(previousReleaseTitle, startIndex);
if (!strictLatest && endIndex === -1) {
endIndex = this.changelogContent.length;
const { isIncrement } = this.config;
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 is where the new code starts. Everything between if (changelog) return changelog; and const { isIncrement } = this.config; is just to satisfy throwing an error if strictLatest is true and the changelog has never had a release before.

test.js Outdated Show resolved Hide resolved
@webpro webpro merged commit 22b4e2e into release-it:master Aug 10, 2022
@webpro
Copy link
Contributor

webpro commented Aug 10, 2022

Thanks for all the work and the feedback, @k2snowman69! I'm going to look into your feedback seriously, but I don't have the time for this now. Just released this in v3.1.0.

@k2snowman69
Copy link
Contributor Author

Glad to help! Let me know if you want to discuss about any of my feedback further. I've got a packed schedule now-a-days but I'll do my best to carve out some time.

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

Successfully merging this pull request may close these issues.

Entire changelog used when using --no-increment flag
2 participants