-
-
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
Migrate to TypeScript #59
Conversation
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.
Not quite through it all yet, but it looks good so far!
I did notice that types remain in most of the JSDoc comments. Now that we have real types, those are redundant and should probably be removed.
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
* Handle undefined newChangelogContent in CLI update function Co-authored-by: Mark Stacey <[email protected]>
Co-authored-by: Mark Stacey <[email protected]>
* Represents a formatting error in a changelog. | ||
*/ | ||
export class ChangelogFormattingError extends InvalidChangelogError { | ||
public data: Record<string, 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.
Curious that in cli.ts
, when this is referenced, it's able to infer that it has entries for validChangelog
and invalidChangelog
🤔 I don't quite follow how it knows those properties exist. Some kinda fancy type inference maybe.
src/parseChangelog.ts
Outdated
// TODO: Get rid of typecast? | ||
mostRecentCategory = results[1] as ChangeCategory; |
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.
Curious about your thoughts here. I don't think we can be certain that this will be a valid change category?
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.
Yeah, you're right. I think we should check, and throw if it's invalid.
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.
LGTM!
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.
LGTM!
Migrates the whole project to TypeScript. Care has been taken to change as little as possible, but in some places I had to add behavior-altering code to please
tsc
.At the time of writing, there are a handful of type-related, outstanding TODOs, which I've highlighted with inline comments.