-
Notifications
You must be signed in to change notification settings - Fork 231
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
Search for configuration in config directories (dotfiles, etc) #749
base: main
Are you sure you want to change the base?
Conversation
Searches for: - ~/Library/Application Support/swift-format/config.json - $XDG_CONFIG_HOME/swift-format/config.json - ~/.config/swift-format/config.json
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.
Thanks for the PR, @calebhearth. I think adding an ability to have a global .swift-format
configuration without putting .swift-format
in /
will be useful.
A couple comments regarding the search locations:
- I think we should also support
XDG_CONFIG_DIRS
for a system-wide configuration file isntead of just a user-wide configuration file. - We should also offer default search locations on Windows, the Build Server Protocol uses
%LOCALAPPDATA%\bsp\
and%PROGRAMDATA%\bsp\
and I think we could do the same here. - The search path discovery for BSP servers is implemented in SourceKit-LSP at https://github.com/swiftlang/sourcekit-lsp/blob/4ca25836226a2b28868a42df3a3336fcc481e1c5/Sources/BuildSystemIntegration/ExternalBuildSystemAdapter.swift#L195-L220. I think you should be able to steal a lot of that (especially note that there are methods in Foundation to get the path for the
Application Support
directories). Also, I think it makes sense to follow that precedent for ranking the XDG-dirs vsApplication Support
.
@WuerfelDev opened the PR, but I second the thanks! |
Oh, whoops, sorry. @WuerfelDev thank you for opening the PR 🙏🏽 |
I will make the changes within the next week. However I am unable to test on windows Thanks for the SourceKit link, I'll get some "inspiration" from there |
That’s fine. Just try and do the best you can think of, leaving FIXMEs for things that you know need covering and I can do the rest. |
31460eb
to
7b9586c
Compare
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.
A couple more comments. Also, could you format your changes using swift-format? Otherwise CI will fail with that issue.
Merge remote-tracking branch 'upstream/main' into global-config-files
@ahoppen Thank you so much for guiding me through this feature implementation, being new to swift I've learned a bunch of things along the way |
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.
Thanks. Looking through it again, I just found one more small nitpick, otherwise looking great.
@@ -268,6 +270,64 @@ class Frontend { | |||
} | |||
} | |||
|
|||
// Load global configuration file | |||
// First URLs are created, then they are queried. First match is loaded | |||
var configLocations: [URL?] = [] |
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.
Sorry, I only now noticed: Do we ever add nil
to this array? I don’t think so and we wouldn’t need to do case var location?
in the loop below if we don’t allow optionals in here.
Searches for configuration files (
swift-format/config.json
) in the config directories (~/Library/Application Support
,$XDG_CONFIG_HOME
,.config
).That enables a "global" (userwide) configuration that is stored eg in the dotfiles.
The configuration file is only used when no other configuration file was found