-
Notifications
You must be signed in to change notification settings - Fork 37
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
Simplest way make config file selectable #908
base: master
Are you sure you want to change the base?
Conversation
This should allow e.g. `sbt -Dstryker4s.config.file=mod-x/stryker4s.conf stryker` which _at least_ is better than having to move config files around in the filesystem in order to switch between subprojects.
@@ -75,7 +77,7 @@ object ConfigReader { | |||
/** Attempt to read a config | |||
*/ | |||
def tryRead: Reader.Result[T] = { | |||
log.info(s"Attempting to read config from stryker4s.conf") | |||
log.info(s"Attempting to read config from ${defaultConfig}") |
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.
This will not toString nicely... Needs to separate file name from config object.
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.
Can you put the getProperty
or default in a separate variable to properly log the file location?
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.
Awesome! I think a system property is a good place to put this functionality. Could you add some tests and maybe a couple of lines in the configuration docs? System properties are mutable, so a test shouldn't be too much work with a test fixture. With that, this is ready to merge, I think!
Having more config options as sbt settings sounds nice, and something I've been thinking of for a while, but I think it needs a little more work to properly merge options. And ideally, it'd also work for the Maven plugin
@@ -75,7 +77,7 @@ object ConfigReader { | |||
/** Attempt to read a config | |||
*/ | |||
def tryRead: Reader.Result[T] = { | |||
log.info(s"Attempting to read config from stryker4s.conf") | |||
log.info(s"Attempting to read config from ${defaultConfig}") |
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.
Can you put the getProperty
or default in a separate variable to properly log the file location?
Ok, I'll have a look. Btw what's your typical workflow for testing the plugin locally - do you go through |
Please help me refresh my memory, what will be the difference in behavior, if I have set base-dir to
|
Yes,
On multi-module projects, running |
Note to self: also update the ./README.md |
@cbrunnkvist any update on this? |
I could never get it to compile once I had made some change. Most likely something underlated e.g. caused by bloop / metals whatever. I'm going to give it a sec...third shot. |
What it does
Makes the config file path less hardcoded, e.g. allows
sbt -Dstryker4s.config.file=mod-x/stryker4s.conf stryker
. Not sure if we should go the "make it a parameter of the sbt task"-route, but at least this is a much less invasive fix, and is already much better than having to move config files around in the filesystem, just in order to switch between subproject runs.How it works
Not sure if it works - this PR comes straight from the GitHub file editor ;)