Skip to content

package rename changelog validation#157

Merged
kanthesha merged 17 commits intomainfrom
package-rename-validation
Oct 17, 2023
Merged

package rename changelog validation#157
kanthesha merged 17 commits intomainfrom
package-rename-validation

Conversation

@kanthesha
Copy link
Copy Markdown
Contributor

@kanthesha kanthesha commented Oct 6, 2023

In the current released version, if we rename the package and would like to preserve the changelog, validation is failing for the format. Which is expected as package name doesn't match.

These changes will help to preserve the old package release/tag history and also works fine with new package release/s when we run the validation.
It addresses

  1. Changelog validation working fine for the old package name in the entries and also for the new package release entries
  2. An option to configure Old Tag Prefix and Old Package Version

original tag prefix and original latest version can be configured in package.json

"autoChangelog": {
    "packageRename": {
      "originalLastestVersion": "5.0.1",
      "originalTagPrefix": "json-rpc-middleware-stream@"
    }
  },

@kanthesha kanthesha marked this pull request as ready for review October 6, 2023 15:58
@kanthesha kanthesha requested a review from a team as a code owner October 6, 2023 15:58
MajorLift
MajorLift reviewed Oct 6, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 6, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 6, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 6, 2023
View reviewed changes
@MajorLift
Copy link
Copy Markdown
Contributor

MajorLift commented Oct 6, 2023 •
edited
Loading

Looks good overall, especially with how quickly you got this to work!

Maybe you could add a barebones test case in changelog.test.ts with a mock renamed package to ensure that stringifyLinkReferenceDefinitions doesn't break in the future?

@MajorLift MajorLift added the enhancement New feature or request label Oct 6, 2023
MajorLift
MajorLift reviewed Oct 7, 2023
View reviewed changes
kanthesha and others added 5 commits October 9, 2023 10:52
@kanthesha @MajorLift
Update src/changelog.ts
9d93abb 
removing redundant null check.

Co-authored-by: Jongsun Suh <34228073+MajorLift@users.noreply.github.com>
@kanthesha @MajorLift
Update src/changelog.ts
96cb711 
Co-authored-by: Jongsun Suh <34228073+MajorLift@users.noreply.github.com>
@kanthesha
validation test added for renamed package changelog
2ba442d
@kanthesha @MajorLift
Update src/changelog-config.ts
daf65a5 
Co-authored-by: Jongsun Suh <34228073+MajorLift@users.noreply.github.com>
@kanthesha @MajorLift
Update src/changelog-config.ts
694ac81 
Co-authored-by: Jongsun Suh <34228073+MajorLift@users.noreply.github.com>
mcmire
mcmire reviewed Oct 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if using a config file is the right way to go here? I've left some comments below.

MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
src/changelog.ts Outdated
Comment on lines +357 to +359
readonly #versionBeforePkgRename: string | undefined;

readonly #tagPrefixBeforePkgRename: string | undefined;
Copy link
Copy Markdown
Contributor

@MajorLift MajorLift Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistent typing for optional properties.

Suggested change
readonly #versionBeforePkgRename: string | undefined;
readonly #tagPrefixBeforePkgRename: string | undefined;
readonly #versionBeforePkgRename?: string;
readonly #tagPrefixBeforePkgRename?: string;

Copy link
Copy Markdown
Contributor

@mcmire mcmire Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An optional property and a property that can be undefined isn't the same thing in TypeScript. An optional property may or may not be set, but it looks like we do set this property in the constructor — it just might be undefined. So this one seems right to me, but does that make sense to you?

Copy link
Copy Markdown
Contributor

@MajorLift MajorLift Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like | undefined better than ?: here, since undefined is explicitly being passed in with these two variables, and not making them optional will prevent the code from breaking if exactOptionalPropertyTypes is turned on.

My concern is around consistent typing for the other properties in these files that are defined as optional. exactOptionalPropertyTypes would enforce that consistency.

MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
src/changelog.ts Outdated
Comment on lines +383 to +384
versionBeforePkgRename?: string | undefined;
tagPrefixBeforePkgRename?: string | undefined;
Copy link
Copy Markdown
Contributor

@MajorLift MajorLift Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistent typing for optional values. With exactOptionalPropertyTypes disabled, the | undefined is redundant.

Suggested change
versionBeforePkgRename?: string | undefined;
tagPrefixBeforePkgRename?: string | undefined;
versionBeforePkgRename?: string;
tagPrefixBeforePkgRename?: string;

MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
MajorLift
MajorLift reviewed Oct 12, 2023
View reviewed changes
@kanthesha
Copy link
Copy Markdown
Contributor Author

kanthesha commented Oct 12, 2023 •
edited
Loading

@MajorLift I see you have added [] inconsistently around the params and few other changes in couple of places, which are not related to this PR! And if required, it can stay separate, which can be independently reviewed.

@MajorLift
Copy link
Copy Markdown
Contributor

MajorLift commented Oct 12, 2023 •
edited
Loading

Yes, [] is the JSDoc syntax for optional params (https://jsdoc.app/tags-param.html#optional-parameters-and-default-values). They could replace the "An optional, which is required only..." comments although I didn't remove those.

The fixes adding [] were cluttering up the feed and there's no variation between them so I thought I'd consolidate them into a single commit.

mcmire
mcmire reviewed Oct 12, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in reviewing. As you can see I left a lot of comments. Try not to be overwhelmed 😅 I think as a whole this implementation makes sense. A lot of the comments are minor.

@MajorLift MajorLift mentioned this pull request Oct 13, 2023
Closed
2 tasks
mcmire
mcmire reviewed Oct 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have another suggestion for how we can make the code match the intended usage.

@mcmire mcmire mentioned this pull request Oct 16, 2023
Closed
mcmire
mcmire reviewed Oct 16, 2023
View reviewed changes
mcmire
mcmire reviewed Oct 16, 2023
View reviewed changes
mcmire
mcmire reviewed Oct 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one more suggestion around removing the brackets for the optional arguments, but this otherwise looks good to me.

kanthesha and others added 2 commits October 17, 2023 18:26
@kanthesha @mcmire
Update src/changelog.ts
c553d22 
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
@kanthesha @mcmire
Update src/changelog.ts
3c4c682 
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
mcmire
mcmire approved these changes Oct 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@kanthesha kanthesha merged commit a9594ab into main Oct 17, 2023
@kanthesha kanthesha deleted the package-rename-validation branch October 17, 2023 17:33
@cryptodev-2s cryptodev-2s mentioned this pull request Oct 17, 2023
Merged
@mcmire mcmire mentioned this pull request Oct 19, 2023
Closed
@MajorLift MajorLift mentioned this pull request Oct 25, 2023
Closed
This was referenced Dec 16, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MajorLift MajorLift MajorLift left review comments

@mcmire mcmire mcmire approved these changes

+1 more reviewer

@legobeat legobeat legobeat left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kanthesha @MajorLift @mcmire @legobeat