Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rescript tool tweaks #7160

Merged
merged 9 commits into from
Nov 20, 2024
Merged

Rescript tool tweaks #7160

merged 9 commits into from
Nov 20, 2024

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Nov 15, 2024

No description provided.

@nojaf nojaf changed the title Use JSON.t in decodeFromJson Rescript tool tweaks Nov 15, 2024
@nojaf
Copy link
Contributor Author

nojaf commented Nov 18, 2024

@cknitt, did you have a chance to check the failing build here?

@cknitt
Copy link
Member

cknitt commented Nov 18, 2024

Hmm, one thing is that the buildjet runners are still not working.

The other is this

Run opam exec -- dune build --display quiet --profile static
Error:
/home/runner/work/rescript-compiler/rescript-compiler/_opam/lib/ocaml/Makefile.config:
No such file or directory

which occurs on all the other runners. I have not that seen before, and it does not seem related to any changes in this PR.

Will look into it a bit more tomorrow.

@nojaf nojaf marked this pull request as ready for review November 19, 2024 16:57
@nojaf
Copy link
Contributor Author

nojaf commented Nov 20, 2024

Hello @cknitt, could you review this one please?
Once merged, I would like to have another alpha with this.
I want to verify the move of @rescript/tool works as expected.
And I want my original change (let decodeFromJson: JSON.t => doc) to be published.
Thanks!

@nojaf nojaf requested a review from cknitt November 20, 2024 13:00
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM! But maybe @zth who knows more about rescript-tools should have a look, too?

@nojaf
Copy link
Contributor Author

nojaf commented Nov 20, 2024

I'm not against that, but it did kinda happen in rescript-lang/rescript-vscode#1043

@cknitt
Copy link
Member

cknitt commented Nov 20, 2024

Ah ok, sorry! Good to go then, I will merge.

@cknitt cknitt merged commit a935b5d into rescript-lang:master Nov 20, 2024
20 checks passed
@nojaf
Copy link
Contributor Author

nojaf commented Nov 20, 2024

Thanks a bunch @cknitt!
Now, could I persuade you to release another alpha for this 😇?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants