-
-
Notifications
You must be signed in to change notification settings - Fork 354
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
Introduce mandatoryScalacOptions
to keep system scalacOptions
#1428
Conversation
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 agree this is a very useful new setting/target. Yet, I don't like the chosen name. The suffix "internal" doesn't transport any meaning or purpose. Can you elaborate. Something like "platformScalacOptions" came to my mind, but I'm very open for better suggestions.
I used |
d638733
to
0646a91
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.
The allScalacOptions
is nice. It also aligns with e.g. allSources
.
I still struggle with the name. What about mandatoryScalacOptions
. It reflects it own current scaladoc and tells it's purpose.
Cool! Also cool that it aligns since I didn't think about
I like it! Going to change it to |
a503217
to
892441b
Compare
It's easy to override scalacOptions in a Scala.js + Scala 3 module. When you do so, the compiler silently fails to compile Scala.js files. This is a source of errors for Mill users that can be mitigated by defining a separate list of compile options which is used internally to add these mandatory flags.
It's important that all consumers use both the user defined options and internalScalacOptions. So we define `allScalacOptions` for consumers.
Improve Scaladoc in allScalacOptions and scalacOptions
892441b
to
95e4727
Compare
I am now thinking, what if we change |
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.
Only some minor text suggestions.
I don't think this is a good idea. Although we're designing an API here, we also try to build a convenient build tool, and in such a scenario pragmatic solutions are in most cases preferred over lengthy and well-constructed code. You never know how this is going to be used. E.g. if I'm going to need the same options in another module and some visibility prohibits it that would be rather inconvenient. I don't see any big issues with having the |
internalScalacOptions
to keep system scalacOptions
mandatoryScalacOptions
to keep system scalacOptions
Co-authored-by: Tobias Roeser <le.petit.fou@web.de>
Co-authored-by: Tobias Roeser <le.petit.fou@web.de>
This alone is already a more than legit reason to leave it public. I imagine users that are used to check this value from the command line and then, subtlety, can't. |
@lolgab Thank you! |
It's easy to override scalacOptions in a Scala.js + Scala 3 module.
When you do so, the compiler silently fails to compile Scala.js files.
This is a source of errors for Mill users that can be mitigated by
defining a separate list of compile options which is used internally
to add these mandatory flags.