Conversation
nhuurre
left a comment
nhuurre
left a comment
There was a problem hiding this comment.
The other PR had several files where the only change is open Core -> open Base. If a file doesn't need anything from Core then don't open Core.
| let ( ^^ ) = Stdlib.( ^^ ) | ||
| let ( ** ) = Stdlib.( ** ) | ||
| let string_of_int = Stdlib.string_of_int | ||
| let int_of_string = Stdlib.int_of_string | ||
| let int_of_string_opt = Stdlib.int_of_string_opt | ||
| let float_of_string = Stdlib.float_of_string | ||
| let float_of_string_opt = Stdlib.float_of_string_opt | ||
| let float_of_int = Stdlib.float_of_int | ||
| let exit = Stdlib.exit |
There was a problem hiding this comment.
I'd rather not add these.
- we already use
Int.to_string,Int.of_string, andFloat.of_stringin some places. It's more consistent to use those everywhere instead of sometimes callingStdlib.string_of_intetc. (^^),(**),int_of_string_opt,float_of_string_opt, andexitare only used once each so defining them here doesn't reduce the diff. (Also if we're usingFloat.of_stringthen maybe we should useFloat.of_string_optinstead ofStdlib.float_of_string_opt. Is there any difference between those?)- that leaves only
float_of_intbut it's not that much work to change it toFloat.of_int, there's only four calls.
There was a problem hiding this comment.
I'm happy to replace the _to_/_of_ with the module versions, but I'd rather leave ^^/**/exit -- I think shadowing such basic functionality in the standard library is a dumb move on Base's part, which I suspect at least someone at JS agrees with since they don't do it in Core, despite re-exporting everything else in Base
| module Expr_set = struct | ||
| type t = Set.M(Expr.Typed).t |
There was a problem hiding this comment.
If the idea is to minimize the diff, shouldn't this module be inside Expr.Typed? Is there a reason it has to be defined here?
There was a problem hiding this comment.
It doesn't have to be, but this is the only file that actually used it, and writing down the signature in Expr.mli was a bit verbose/annoying
|
I'd also rather audit whether Core needs to be opened at the top of files in a separate PR, because I suspect the Stdlib improvements in OCaml 5 actually mean a lot of our files don't need Core or Base any longer, but that's a more involved project |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1643 +/- ##
==========================================
+ Coverage 91.44% 91.48% +0.04%
==========================================
Files 66 67 +1
Lines 9978 10009 +31
==========================================
+ Hits 9124 9157 +33
+ Misses 854 852 -2
🚀 New features to boost your workflow:
|
|
How does this PR affects the size of the stanc.js file ? |
A pretty decent amount: ~6mb reduction in unminified/debug bundle, ~700kb reduction in minified/release |
|
I
And in percentage ? |
| @@ -0,0 +1,6 @@ | |||
| (library | |||
| (name core) | |||
There was a problem hiding this comment.
This name probaby mean it wont be able to link together with core. I dont know if its an issue for you here
There was a problem hiding this comment.
This was a feature to me, since it meant I couldn’t accidentally pull it back in somewhere :)
I am intending this to be a temporary solution to unblock our downstream projects before doing a more serious audit of whether we even really need Base. Stdlib has gotten quite a lot better in 5.x as far as I can tell
~15% reduction |
4 participants
This should resolve #1640.
To make the patch as minimal as possible, I added our own
Core.mlmodule which shadows all theopen Cores we have, which just uses base and a few other small changes for compatibility. This means we could rather seamlessly return to using core once 0.18 (which removed the weird Temporal polyfill) is released, if we wishedSubmission Checklist
Release notes
Replace this text with a short note on what will change if this pull request is merged. This will be included in the release notes.
Copyright and Licensing
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)