-
Notifications
You must be signed in to change notification settings - Fork 187
fix: envconfig, use OS-specific config file paths #2762
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
Conversation
| if (osName != null) { | ||
| String osNameLower = osName.toLowerCase(); | ||
| if (osNameLower.contains("mac")) { | ||
| return userDir + "/Library/Application Support/temporalio/temporal.toml"; |
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.
nit: Not a big deal, but it's preferable to avoid building paths by simply concatenating raw strings.
| return userDir + "/Library/Application Support/temporalio/temporal.toml"; | |
| return Paths.get(userDir, "Library", "Application", "Support", "temporalio", "temporal.toml").toString(); |
Same for other below.
| * Get the default config file path based on the operating system: | ||
| * | ||
| * <ul> | ||
| * <li>macOS: $HOME/Library/Application Support/temporalio/temporal.toml |
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 comment is on a private method, where it is less likely to be found by a user that wonders where ClientConfig is storing its data, which is a typical and legitimate question for a user. I'd suggest moving this bullet list to the class's javadoc instead, then just add a reference to that here.
mjameswh
left a comment
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.
Minor comments, nothing blocking.
What was changed
Envconfig fix, to use OS-specific config file paths
Why?
This is the correct behaviour (as documented)
Closes Environment Configuration does not read the correct file path on macOS #2754
How was this tested:
Added test
Any docs updates needed?
No