refactor: port parse to typescript #764
Conversation
byCedric
force-pushed
the
refactor/parse-to-ts
branch
from
278ad68
to
46596a3
Compare
|
@marionebl I'm not really happy adding |
|
Done, let me know what you think 😄 As I said, I think it's better adding types during the port-period in this instance. If we don't do this we have to add a lot of explicit types in all packages using this, especially the rules. |
| const message = '#1 some subject PREFIX-2'; | ||
| const {references} = await parse(message, undefined, { | ||
| issuePrefixes: ['PREFIX-'] | ||
| }); | ||
|
|
||
| t.falsy(references.find(ref => ref.issue === '1')); | ||
| t.deepEqual(references.find(ref => ref.issue === '2'), { | ||
| expect(references.find(ref => ref.issue === '1')).toBeFalsy(); |
There was a problem hiding this comment.
If we don't add types to the parser output, you can't use ref => ref.issue === '1' like this. You need to add some types for this, everytime you use it. That's why I added it in the second commit. So this has to change to:
expect(references.find((ref: any) => ref.issue === '1')).toBeFalsy();
There was a problem hiding this comment.
Adding an optional type parameter defaulting to unknown should do the trick. Further discussion at the signature of parse
There was a problem hiding this comment.
I'm not sure if I understand this fully, do you want to add a different typing to the existing references property from interface Commit? Or did you mean something else? 😄
| process.cwd(), | ||
| './fixtures/parser-preset/conventional-changelog-custom' | ||
| const changelogOpts: any = await importFrom( | ||
| __dirname, |
There was a problem hiding this comment.
This was a bit weird because we are running Jest from the root of the monorepo process.cwd seems to be set to that folder. In this case, we need to scope it from within this package. This works, but not sure if this is the best solution.
There was a problem hiding this comment.
I think it is fine. We might want to use fixturez for this kind of thing to decouple test files from exact relative fixture paths. If you feel eager you could give it a try here, otherwise let's go forward as is.
There was a problem hiding this comment.
I think this is important for @commitlint/load, was doing some work on that but did not finish in time. In this instance, it might be a bit of an overkill? 😬
|
@marionebl They look great! I'll update the PR in a moment 😄 |
|
One thing that was on my mind is the naming of the interfaces. I can imagine renaming |
byCedric
force-pushed
the
refactor/parse-to-ts
branch
from
cc7373a
to
8f08e67
Compare
|
Ok, I think I processed all the comments. I still have some "random thoughts" but they are not necessary for now. Let me know what you think, I might have some time left tomorrow 😄 |
|
@marionebl Are the changes I applied after your feedback good to go? Or does it still require some additional changes? 😄 |
|
I chatted with @marionebl, it was a go 😄 |
Description
Part of #659.
Motivation and ContextUsage examplesHow Has This Been Tested?Types of changes
Checklist: