-
Notifications
You must be signed in to change notification settings - Fork 129
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
Don't compute "hash data" for options that are not used by Bloop #648
Don't compute "hash data" for options that are not used by Bloop #648
Conversation
Make that more explicit via `notForBloopOptions: PostBuildOptions`.
dd0f00a
to
f946e6f
Compare
@@ -38,8 +38,7 @@ final case class BuildOptions( | |||
internal: InternalOptions = InternalOptions(), | |||
mainClass: Option[String] = None, | |||
testOptions: TestOptions = TestOptions(), | |||
packageOptions: PackageOptions = PackageOptions(), | |||
replOptions: ReplOptions = ReplOptions() | |||
notForBloopOptions: PostBuildOptions = PostBuildOptions() |
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 we should call it postBuildOptions
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.
Or maybe rename PostBuildOptions
to NotForBloopOptions
? I wanted to make it clear that it shouldn't be used when generating the Bloop config for example.
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.
It is strange that BuildOptions contains PostBuild / NotForBloop. maybe we should Refactor it at some point so we have an Option object that contains BuildOptions (ones that bloop cares for), runtime options (like main class, tests, repl things), package options etc.
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.
That's what I wanted to do initially, something like
final case class AllBuildOptions(
buildOptions: BuildOptions = BuildOptions(),
postBuildOption: PostBuildOptions = PostBuildOptions()
)
I just fear it'll touch many-many files in the codebase.
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, we can live now with notForBloopOptions
and refactor option to AlLBuildOption
at some point
…tusLab#648) Make that more explicit via `notForBloopOptions: PostBuildOptions`.
This PR makes that more explicit via
notForBloopOptions: PostBuildOptions
inBuildOptions
.This hash is only used to make Bloop directories unique, so it doesn't need to take package or repl options into account.