Closed Latyntsev closed 4 years ago
Thanks for the pull request! This looks like an interesting direction but there's a couple of things I wanted to talk about:
CLI tool can't read UserDefault settings that were set up via App
We should probably fix this instead of work around it. I would rather there be one place where "global" configuration goes rather than have to figure out whether to pick between UserDefaults
and a configuration file if both exist.
Sometimes need settings per project with ignoring local settings for automation through the build phase
Right, this sounds useful. I'm sure people may like per-project settings as well.
Aside from that, I'll take a look at the code in a bit. I think there's a change that's causing the tests to fail; do you have any idea what that is? If not, I'll go check it out when I have time.
@saagarjha thank you for feedback
We should probably fix this instead of work around it.
I agree it should be fixed. I was neede local config and I thought, the bug should be fixed in separate PR.
I would rather there be one place where "global" configuration goes rather than have to figure out whether to pick between UserDefaults and a configuration file if both exist.
I see. this is only for CLI.. extension still using "global" config. From what I see, it's normal practice to use a config file for CLI tools.
I could refactor it to use two addition flags instead of the config file:
-p || --parametersAligned
-s || --semicolonsRemoved
example
swimat -v -r -p true -s true ./
swimat -v -r --parametersAligned true --semicolonsRemoved false
WDYT?
Right, this sounds useful. I'm sure people may like per-project settings as well.
In my case I want to fix code formatting for my team at commit time, ignoring local configs. Need a solution that will work on different machines with the same result.
Aside from that, I'll take a look at the code in a bit. I think there's a change that's causing the tests to fail; do you have any idea what that is? If not, I'll go check it out when I have time.
Sorry about that... I will double-check it
@saagarjha could you please check the PR, when you will have time
I see. this is only for CLI.. extension still using "global" config. From what I see, it's normal practice to use a config file for CLI tools.
Right, and that configuration file will happen to be ~/Library/Group\ Containers/com.jintin.swimat.configuration/Library/Preferences/com.jintin.swimat.configuration.plist
. I'm saying we don't need another file.
could refactor it to use two addition flags instead of the config file
I think passing in a configuration file from the command line is fine. Adding every configuration option as a command line flag will get unwieldy quickly.
Also, I just glanced at the code and it seems reasonable. A couple of comments: I like the use of an enumeration for preference keys, but I'm not really a fan of writing our own initializers. Can you figure out a way to keep the compiler-generated ones? Also, I'm curious where you're trying to go with Preferences.shared
.
Once we resolve these, there's a couple of more minor things that we can work on afterwards in as a normal review.
Read the configuration from the file for CLI.
Example:
swimat -c .swimat.json
swimat --config ./.config.swimat.json
by default, the tool will use
.swimat.json
if existconfig example
Motivation