Gentype support #138
Gentype support #138
Conversation
|
Looks good! Would you just add a test for |
There was a problem hiding this comment.
Thanks for contributing @Bushuo - Left a small suggestion regarding the unwraps and representation 🙌
| @@ -157,6 +161,8 @@ pub struct Config { | |||
| pub namespace: Option<NamespaceConfig>, | |||
| pub jsx: Option<JsxSpecs>, | |||
| pub uncurried: Option<bool>, | |||
| #[serde(rename = "gentypeconfig")] | |||
| pub gentype_config: Option<GenTypeConfig>, | |||
There was a problem hiding this comment.
I would probably not have this as an empty struct, but rather just serde::Value - such that it represents some random piece of JSON which we don't care about. Right now it implies it's empty, rather than something we don't care about.
src/bsconfig.rs
Outdated
| .unwrap() | ||
| .parse::<usize>() | ||
| .unwrap() | ||
| >= 11 |
There was a problem hiding this comment.
I'm not sure this cuts it - I think you can do wildcards like: rescript: *, which I think would fail to parse, and that would throw in this case. To really check this, we should probably parse the output of the rescript -v. But, I'm ok to use this for the time being, albeit with some small changes:
- Return
Result<bool>here (probably my preference) - Default to false
- Or write some heavy tests around this
There was a problem hiding this comment.
I see we also hacked this in at some point. But perhaps, while we're touching it, maybe we just fix it up nicely 👌 - If I have some time today, I'll try and conjure up a version as a suggestion
There was a problem hiding this comment.
I will go with the Result, then we can log an error at the call site and decide how to proceed.
In this case without uncurried mode.
|
Thanks for the reviews y'all. I will make adjustments later today. |
This PR adds support for gentype to rewatch.
With the upcoming v11 compiler version it will then generate the appropriate
.tsxfiles.Fixes #102