Conversation
paths/paths.go
Outdated
| panic(err) | ||
| } | ||
| // Recalculate sets exercism paths | ||
| func Recalculate(home, configHome string) { |
There was a problem hiding this comment.
It seems this functions whole purpose now is to setup the Home and ConfigHome and only used within init. What about just dropping the function all together and setting the variables directly on the tests and doing this same logic from init?
There was a problem hiding this comment.
I agree, this function doesn't have a lot of value right now, especially given that we now have other inputs for doing the testing. I think we could justify keeping it if we gave it a better name. Maybe assignConfigHome or resolveConfigHome (if we assign paths.ConfigHome outside the function). if we don't fold it in to init, I don't think it needs to be exported.
Edit: I think I'd vote for keeping this method as it encapsulates a nuanced piece of logic and gives us a simple way to test things.
|
Tests pass. Looks great! |
|
Thank you for the feedback, I'll fix the PR anytime soon. But, to be honest, I'm not very happy about the logic of the To my taste, it feels unnatural, that
during the initialization process. At least, tools like git and others do it this way. |
|
@narqo we are always open for discussion and thanks for bringing up your concern. If I understand you correctly, you mean we should be checking for the existence of the That sounds reasonable to me, would you be open to making those changes? |
Sure, it'd be nice brain training. |
Yeah, same here. |
- respect `$XDG_CONFIG_HOME` environment variable - search for config in common paths on initialisation fix tests for exercism.paths on systems with `$XDG_CONFIG_HOME` In case `$XDG_CONFIG_HOME` is set, tests of `paths` package fail as config home was not mocked properly.
narqo
changed the title
|
🆙 I've updated the logic of I believe the changes match to the current behaviour of the CLI. |
|
I believe the current behavior has three possibilities in this order of priority:
|
| } | ||
| } | ||
|
|
||
| func TestXDGConfig(t *testing.T) { |
There was a problem hiding this comment.
This test is useless now, as the behaviour of the paths.Config() only depends on ConfigHome, which was tested above.
Yeah, thank you, I've forgotten to mention this. So now the priority paths are:
|
|
Thanks for the work on this @narqo, I'm going to go ahead and merge this and update the CHANGELOG! |
|
Thank you @narqo! |
|
I'm totally glad I was helpful |
4 participants
Search for config in common paths (e.g.
~/.config/exercism.json, ~/.exercism.json`).In case
$XDG_CONFIG_HOMEis set, tests of paths package fail as config home was not mocked properly.The patch simplifies paths' internals and introduces a more flexible way to mock globals of
pathspackage.