-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
package rename changelog validation #157
Conversation
Looks good overall, especially with how quickly you got this to work! Maybe you could add a barebones test case in |
removing redundant null check. Co-authored-by: Jongsun Suh <[email protected]>
Co-authored-by: Jongsun Suh <[email protected]>
Co-authored-by: Jongsun Suh <[email protected]>
Co-authored-by: Jongsun Suh <[email protected]>
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 wonder if using a config file is the right way to go here? I've left some comments below.
src/changelog.ts
Outdated
readonly #versionBeforePkgRename: string | undefined; | ||
|
||
readonly #tagPrefixBeforePkgRename: string | undefined; |
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.
Consistent typing for optional properties.
readonly #versionBeforePkgRename: string | undefined; | |
readonly #tagPrefixBeforePkgRename: string | undefined; | |
readonly #versionBeforePkgRename?: string; | |
readonly #tagPrefixBeforePkgRename?: string; |
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.
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?
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.
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.
src/changelog.ts
Outdated
versionBeforePkgRename?: string | undefined; | ||
tagPrefixBeforePkgRename?: string | undefined; |
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.
Consistent typing for optional values. With exactOptionalPropertyTypes
disabled, the | undefined
is redundant.
versionBeforePkgRename?: string | undefined; | |
tagPrefixBeforePkgRename?: string | undefined; | |
versionBeforePkgRename?: string; | |
tagPrefixBeforePkgRename?: string; |
@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. |
Yes, The fixes adding |
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.
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.
…ed links code refactor
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 have another suggestion for how we can make the code match the intended usage.
" This reverts commit b51a4fa.
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 have one more suggestion around removing the brackets for the optional arguments, but this otherwise looks good to me.
Co-authored-by: Elliot Winkler <[email protected]>
Co-authored-by: Elliot Winkler <[email protected]>
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.
Looks good to me!
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
original tag prefix and original latest version can be configured in package.json