-
Notifications
You must be signed in to change notification settings - Fork 471
Automatic migrations #7829
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
base: master
Are you sure you want to change the base?
Automatic migrations #7829
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
0baf11f
to
1814366
Compare
…al feature" This reverts commit 59c902c.
d17ce33
to
14e8eab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting complexity, but it's a surprisingly small number of surgical changes to the compiler.
Looks great.
Also, the annotation work is impressive.
|
||
- Add optional `message` argument to `Result.getOrThrow` and improve default error message. https://github.com/rescript-lang/rescript/pull/7630 | ||
- Add `RegExp.escape` binding. https://github.com/rescript-lang/rescript/pull/7695 | ||
- Add `Array.fromString`. https://github.com/rescript-lang/rescript/pull/7693 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
let uri = Uri.fromPath path in | ||
fullFromUri ~uri | ||
|
||
let loadCmtInfosFromPath ~path = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are infos new here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, since we don't need to process the CMT as we do in analysis, this just loads the CMT and accesses the new prop.
| _ -> None) | ||
in | ||
|
||
(* TODO: Validate and error if expected shape mismatches *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll leave this in for now, and do proper errors and validations if/when we decide this should be a first class feature in the language itself. For now it's just an internal migration tool for the library (although anyone can use it of course, at their own risk).
cmt_imports : (string * Digest.t option) list; | ||
cmt_interface_digest : Digest.t option; | ||
cmt_use_summaries : bool; | ||
cmt_extra_info: Cmt_utils.cmt_extra_info; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK yes they are new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually I think I want to move this to a separate sidecar file, probably the one I've added in the Actions PR. But for now (and maybe always) it can live in here, since it has minimal impact.
let get_saved_types () = !saved_types | ||
let set_saved_types l = saved_types := l | ||
|
||
let record_deprecated_used ?deprecated_context ?migration_template ?migration_in_pipe_chain_template source_loc deprecated_text = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these going to be reversed at some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reversed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reversed?
It's constructed backwards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't follow 😄
let path, desc = | ||
Typetexp.find_value | ||
?deprecated_context: | ||
(match deprecated_context with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why an optional then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go through again and see if we always pass it (and if so make it non-optional).
Supersedes #7693
Left TODO for this first iteration
.resi
files as wellError on invalid config etcWe'll do this when/if we make this an actual first class feature to be used outside of the stdlibConsider if we want to deprecate the functions in Belt that are 1:1/fully covered in the new stdlib. If so, add deprecations and migrations for them as wellWe'll do Belt in 12.1 or later.Decide on what to do with: