Add types to options.js #126
Conversation
|
@jfmengels all of the comments should be addressed 🙂 |
| if (flag.alias) { | ||
| object[flag.name] = flag.alias; |
There was a problem hiding this comment.
There has to be a way to tell TS that flag.alias is always false, right? 🤔
(Not that I necessarily want to remove it, I'm mostly intellectually curious 😅 )
There was a problem hiding this comment.
Is this what you mean? https://github.com/total-typescript/ts-reset#make-filterboolean-filter-out-falsy-values
There was a problem hiding this comment.
Yes, that's what I meant. I don't know how smart ts-reset makes filter in this case, but I'd imagine that in this case TS will not remember that flag.alias is non-falsy. If it does, then yes, otherwise, it's so close 😁
There was a problem hiding this comment.
Fixed it locally, I tossed it into a gist:
https://gist.github.com/lishaduck/8cabda9ae913535f702a4376712d4608
I'd just give a patch, but I'm tweaking some other types. I'll PR soon (unless I forget 😉) EDIT: #155
I'm planning on rebasing that on this.
It's a patch now!
|
There are still a few errors in the regular |
|
I can make sense for it be |
| export type OptionsForRunningElmReview = Omit<Options, 'elmJsonPath'> & { | ||
| elmJsonPath: Path; | ||
| }; | ||
|
|
There was a problem hiding this comment.
If we think more types are going to have to change for running elm-review, we could also abstract this a little:
type OverriddenFieldsForRunningElmReview = { elmJsonPath: Path, ... }
export type OptionsForRunningElmReview = Omit<Options, keyof OverriddenFieldsForRunningElmReview> & OverriddenFieldsForRunningElmReviewAlso, this name is pretty long, but I couldn't think of anything better 😅
| if (!elmModulePath || !options.elmJsonPath) { | ||
| AppState.exitRequested(1); | ||
| return; | ||
| } | ||
|
|
||
| const optionsWithElmJsonPath = {...options, elmJsonPath: options.elmJsonPath}; |
There was a problem hiding this comment.
For now, we just exit if elmJsonPath is null. We could have better errors if you want (if so, we could wrap this in a function and just call it where needed) 🙂
There was a problem hiding this comment.
Sorry for long delay! I was on a much needed vacation break!
I think the PR looks pretty good in general. Looking at the tests (npx jest to run those locally), some behavior has changed.
-
The check for
elm.jsonis done earlier than the check for bad flags (unknownCheck), and now that triggers different and less helpful errors than before. -
Somehow the suppressions behave differently compared to before?
Sorry again for the delay, and I hope it's not too much of a bother (resuming) working on this. If it is, simply let me know and I'll try to get this across the finish line. I appreciate all of your help @henriquecbuss ❤️ 😊
|
No worries! A much deserved break, for sure. I'd love to keep working on this, but a lot of stuff (work + school) is going on at once, so I will not have time to continue working on this for a while (probably at least a month). It's fine if you want to finish this PR yourself, as that is quite a long time, but it's also fine if you want to wait for me to finish it (just know it will take a while). Hopefully I can contribute more as soon as I get more free time 🙂 |
|
Oh, @jfmengels, this can be closed. You made the types changes already, the only changes left break the snapshots, as you observed. I plan on investigating it, but as the main purpose is complete I think it can be closed. |
|
Alright, thank you for looking into this @lishaduck. And thank you for the original work @henriquecbuss 🙏 |
This PR is a step towards #125, and adds types to
options.js.It also fixes a
@ts-expect-errorin the same file -- that's why some other files are touched.Thanks for the project, glad I can help in some way 🙏