-
Notifications
You must be signed in to change notification settings - Fork 463
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
Make build -with-deps a default behaviour #6350
Make build -with-deps a default behaviour #6350
Conversation
c7231fe
to
0e27a34
Compare
Fixing tests 🧑💻 |
49930c5
to
e6fc780
Compare
@cknitt @christian-schulze It's ready for review 😁 |
LGTM! @cristianoc What do you think? |
(I fear you'll need to rebase again now that I have merged #6352.) |
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.
Looks good.
The only thing the description is not explicit about is whether this is a breaking change in some cases.
00f40a7
to
e8ee648
Compare
Hmm, there is an error running the tests now that I don't quite understand. |
There's an extra line break in the error, I'll fix |
- Did you include the file's directory to the "sources" in bsconfig.json? |
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.
Highlighting that I've changed the text a little bit.
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.
Then we should not forget to change it again when we change the names of the fields for rescript.json. Since we probably will ditch the bs-
prefix here as well.
Or you phrase it more generally, like:
Did you add it to the dependencies in the compiler configuration file?
(which is arguably a bit more confusing)
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.
Did you add it to the dependencies in the compiler configuration file?
This one is good if we are going to keep both bsconfig.json
and rescript.json
for a while. Otherwise, it should be changed together with replacing bsconfig.json
with rescript.json
in all places where it mentioned.
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 am indifferent. What do you think @cknitt @cristianoc @zth ?
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 think it's fine to leave it like it is. We'll do a search replace for bsconfig -> rescript anyway when it's time.
Here's the discussion from the forum about the change https://forum.rescript-lang.org/t/proposal-make-build-with-deps-a-default-behaviour/3832
Implementation
-with-deps
parsing even though it doesn't do anything anymore, so there's no `unknown option: '-with-deps' error. Removing it completely in the next major version might be a good idea.ocamlformat
work, so if it should work for the project, please let me know how.