search

jfmengels / node-elm-review

CLI for elm-review
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
BSD 3-Clause "New" or "Revised" License
48 stars 25 forks source link

Add types to options.js #126

Closed henriquecbuss closed 5 months ago

henriquecbuss commented 1 year ago

This PR is a step towards #125, and adds types to options.js.

It also fixes a @ts-expect-error in the same file -- that's why some other files are touched.

Thanks for the project, glad I can help in some way 🙏

henriquecbuss commented 1 year ago

@jfmengels all of the comments should be addressed 🙂

henriquecbuss commented 1 year ago

There are still a few errors in the regular tsconfig.json configuration, related to options.elmJsonPath being nullable. I'm not exactly sure how to go about this... does it make sense for it to be nullable? Is it even possible to run elm-review without an elm.json? Maybe we should error out if we can't find it?

jfmengels commented 1 year ago

I can make sense for it be null, when you're running commands like elm-review --help or init for instance. But for the review part, it should never be null. When it is, we will be throwing an error somewhere. I'm not sure what's the best way to prove that to TS. I guess casting could be an okay thing for now, until we figure out something better (if you don't want to scratch your head too much). Other ideas welcome, I'm not super familiar with TS.

henriquecbuss commented 1 year ago

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 🙂

lishaduck commented 5 months ago

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.

jfmengels commented 5 months ago

Alright, thank you for looking into this @lishaduck. And thank you for the original work @henriquecbuss :pray: