This repository was archived by the owner on Apr 24, 2021. It is now read-only.
Start removing let%foo #87
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- The let%try and others ended up swallowing lots of important production errors (surprise... =/) - The transforms in their current state don't give an error message location (partially due to usage of metaquot. Meta yak shaving). - This makes the build depend on OMP, ppxlib and others. Giant deps, giant build times. - But seriously, if even Cristiano has problem writing proper option and result handling in production because of `let%` stuff then what chance do most others have. Anyway I'm gonna remove it because this seems to be a blocker for cleaning up the codebase's other parts... First step is to build the same monad ppx as an executable; I'll directly run this through the source files using `unmonad.sh`, print them back to Reason syntax, and manually clean up some stuff. See #86, which is converted by the script. Using the ppx script itself ensures that we don't cause conversion bugs. After this PR, I'm thinking of directly making the ppx transform the `let%` stuff into regular switch statements, and print out those.
|
Merging. No bug since this is additive |
|
@chenglou there are a lot of warnings like this when building: I guess that's just temporary during the removal. |
|
Oh they were all coming from that file. Just silenced the warnings for ppx2 in master. |
chenglou
added a commit
that referenced
this pull request
Apr 7, 2021
Follow-up of #87 Now we can properly add error handling to these `None`/`Error(_)` later
chenglou
added a commit
that referenced
this pull request
Apr 7, 2021
chenglou
added a commit
that referenced
this pull request
Apr 7, 2021
chenglou
added a commit
that referenced
this pull request
Apr 7, 2021
chenglou
added a commit
to chenglou/rescript-editor-support
that referenced
this pull request
Apr 24, 2021
- The let%try and others ended up swallowing lots of important production errors (surprise... =/) - The transforms in their current state don't give an error message location (partially due to usage of metaquot. Meta yak shaving). - This makes the build depend on OMP, ppxlib and others. Giant deps, giant build times. - But seriously, if even Cristiano has problem writing proper option and result handling in production because of `let%` stuff then what chance do most others have. Anyway I'm gonna remove it because this seems to be a blocker for cleaning up the codebase's other parts... First step is to build the same monad ppx as an executable; I'll directly run this through the source files using `unmonad.sh`, print them back to Reason syntax, and manually clean up some stuff. See rescript-lang#86, which is converted by the script. Using the ppx script itself ensures that we don't cause conversion bugs. After this PR, I'm thinking of directly making the ppx transform the `let%` stuff into regular switch statements, and print out those.
chenglou
added a commit
to chenglou/rescript-editor-support
that referenced
this pull request
Apr 24, 2021
…-lang#89) * Make the unmonad script turn the let% into `switch` instead Follow-up of rescript-lang#87 Now we can properly add error handling to these `None`/`Error(_)` later * Check in the tests on 2 files Supercedes rescript-lang#86
chenglou
added a commit
to chenglou/rescript-editor-support
that referenced
this pull request
Apr 24, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
switch. It's not possible that only 28% needed proper error handling. This explains our previous point.let%stuff then what chance do most others have.Anyway I'm gonna remove it because this seems to be a blocker for cleaning up the codebase's other parts...
First step is to build the same monad ppx as an executable; I'll directly run this through the source files using
unmonad.sh, print them back to Reason syntax, and manually clean up some stuff.See #86, which is converted by the script. Using the ppx script itself ensures that we don't cause conversion bugs.
After this PR, I'm thinking of directly making the ppx transform the
let%stuff into regular switch statements, and print out those.