chore: Full conversion to typescript (#29)#74
chore: Full conversion to typescript (#29)#74SharpSeeEr wants to merge 20 commits intoabsolute-version:masterfrom
Conversation
chore: update deps / package.json scripts; add `tsup` build tool; feat: new config API (WIP)
refactor: cleaner types in command.ts
TimothyJones
left a comment
TimothyJones
left a comment
There was a problem hiding this comment.
I took a quick look. Impressive amount of work!
I'm not clear on what some of the TODO comments mean - could you perhaps update them to become tasks rather than notes? Am I right in thinking that they would need to be addressed before release?
Also, looks like there's only one test converted, is that right?
| // // default: defaults[propName] ? defaults[propName] : property.default, | ||
| // // group: 'Preset Configuration:', | ||
| // }); | ||
| // }); |
There was a problem hiding this comment.
There are a lot of comments in this file - are they supposed to be still commented?
There was a problem hiding this comment.
It was the end of my day and I wanted to get the changes pushed up to get other eyes on them so I was in a hurry and didn't do any cleanup. :)
There was a problem hiding this comment.
Oh, also, the commented code at the bottom of the file was meant to dynamically add the properties from the spec to yargs, so if they changed anything in the spec the yargs config would automatically get updated. So I intentionally left this here to make sure that everyone was okay with removing it.
It doesn't really help, as the code depends on having those properties, and if the spec changed we would still have to update the code.
As for all the commented default: lines, I did that because the --help output looked awful. I think a better solution would be to add an .epilogue() with a link to the docs that explain what the default values are.
| } | ||
| } else { | ||
| config = JSON.parse(readFileSync(configPath)) | ||
| return {} |
lib/detect-package-manager.ts
Outdated
| } | ||
|
|
||
| const detectPMByLockFile = async (cwd) => { | ||
| const detectPMByLockFile = async (cwd: string) => { |
There was a problem hiding this comment.
This should probably be exported too, so the interface is the same
|
Looks like the build failure is real, too: https://github.com/absolute-version/commit-and-tag-version/actions/runs/4672719409/jobs/8275798247?pr=74#step:9:174 |
|
Sorry I didn't get to your other PR in time - I wanted to comment and say that incremental PRs with settings updates would be a good approach - we could potentially have done the tests + lint first without touching the implementation, then merged that in to the typescript branch. That comment seems obsolete now, though 😂 |
|
I 100% agree that incremental PR's are the way to go. In my rush I opened this against master rather than the typescript conversion branch, which would allow more incremental PR's. I also screwed up by not renaming each file and then committing the change before making modifications so it shows as a rename instead of a delete and add. As far as the build goes I didn't even try. I knew it wouldn't build at this stage. I'll work on converting the tests next. |
|
I'm afraid I'm not really sure what to do with this one - I don't think we can really call it a full conversion unless it builds. What state is it currently in? I might convert it to a draft while the build doesn't run. |
3 participants
There are some TODO: comments scattered around in places where I honestly had no idea what to do.