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

Make build -with-deps a default behaviour #6350

Merged
merged 9 commits into from
Aug 19, 2023

Conversation

DZakh
Copy link
Contributor

@DZakh DZakh commented Aug 16, 2023

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

  1. I've kept the -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.
  2. It's my first contribution to the compiler, and I'm excited to do more stuff in the future. I had to overcome a little barrier that it's something scary to do.
  3. I couldn't make the ocamlformat work, so if it should work for the project, please let me know how.

@DZakh DZakh force-pushed the deprecate-with-deps-arg branch 2 times, most recently from c7231fe to 0e27a34 Compare August 16, 2023 09:34
@DZakh
Copy link
Contributor Author

DZakh commented Aug 16, 2023

Fixing tests 🧑‍💻

@DZakh
Copy link
Contributor Author

DZakh commented Aug 16, 2023

@cknitt @christian-schulze It's ready for review 😁

@cknitt
Copy link
Member

cknitt commented Aug 16, 2023

LGTM! @cristianoc What do you think?

@cknitt
Copy link
Member

cknitt commented Aug 16, 2023

(I fear you'll need to rebase again now that I have merged #6352.)

Copy link
Collaborator

@cristianoc cristianoc left a 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.

jscomp/bsb_exe/rescript_main.ml Outdated Show resolved Hide resolved
scripts/ciTest.js Outdated Show resolved Hide resolved
@cknitt
Copy link
Member

cknitt commented Aug 18, 2023

Hmm, there is an error running the tests now that I don't quite understand.

@DZakh
Copy link
Contributor Author

DZakh commented Aug 18, 2023

There's an extra line break in the error, I'll fix

Comment on lines +10 to +11
- Did you include the file's directory to the "sources" in bsconfig.json?
Copy link
Contributor Author

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.

Copy link
Contributor

@fhammerschmidt fhammerschmidt Aug 18, 2023

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)

Copy link
Contributor Author

@DZakh DZakh Aug 18, 2023

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.

Copy link
Contributor

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 ?

Copy link
Collaborator

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.

@cknitt cknitt merged commit cc410f2 into rescript-lang:master Aug 19, 2023
14 checks passed
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.

None yet

5 participants