Update config directory resolution#42
Conversation
stephencelis
left a comment
stephencelis
left a comment
There was a problem hiding this comment.
Thanks for looking into this! We are definitely down to merge but could you please address the following?
Sources/pfw/Token.swift
Outdated
| var machineURL: URL { | ||
| pfwDirectoryURL.appendingPathComponent("machine") | ||
| } | ||
|
|
||
| var tokenURL: URL { | ||
| pfwDirectoryURL.appendingPathComponent("token") | ||
| } | ||
|
|
||
| var shaURL: URL { | ||
| pfwDirectoryURL.appendingPathComponent("sha") | ||
| } | ||
|
|
||
| var pfwDirectoryURL: URL { | ||
| pfwDirectoryEnvOverride ?? xdgDirectoryURL ?? defaultPfwDirectoryURL | ||
| } | ||
|
|
||
| private var defaultPfwDirectoryURL: URL { | ||
| @Dependency(\.fileSystem) var fileSystem | ||
| return fileSystem.homeDirectoryForCurrentUser | ||
| .appendingPathComponent(".pfw", isDirectory: true) | ||
| } | ||
|
|
||
| private var pfwDirectoryEnvOverride: URL? { | ||
| let pfwHome = ProcessInfo.processInfo.environment["PFW_HOME"] | ||
| if let path = pfwHome?.trimmingCharacters(in: .whitespacesAndNewlines), !path.isEmpty { | ||
| return URL(fileURLWithPath: path, isDirectory: true) | ||
| } else { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| private var xdgDirectoryURL: URL? { | ||
| let xdgConfigDir = ProcessInfo.processInfo.environment["XDG_CONFIG_HOME"] | ||
| if let path = xdgConfigDir?.trimmingCharacters(in: .whitespacesAndNewlines), !path.isEmpty { | ||
| return URL(fileURLWithPath: path, isDirectory: true) | ||
| .appendingPathComponent("pfw", isDirectory: true) | ||
| } else { | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you refactor this back to look like mostly the original? These aren't reused helpers and inlining things again should be significantly shorter and easier to see what the difference is.
Sources/pfw/Token.swift
Outdated
| } | ||
|
|
||
| private var xdgDirectoryURL: URL? { | ||
| let xdgConfigDir = ProcessInfo.processInfo.environment["XDG_CONFIG_HOME"] |
There was a problem hiding this comment.
Rather than use ProcessInfo.processInfo.environment directly, and then use getenv and setenv for tests, can you control this dependency? There are a few examples in the Dependencies directory.
|
@tomassliz Just checking in. Are you interested in addressing the above feedback to get this merged? |
|
@stephencelis I’ll address these by the end of this week. I apologize for the delay. |
Prefer PFW_HOME when set. Otherwise use XDG_CONFIG_HOME/pfw, or fall back to ~/.pfw.
tomassliz
force-pushed
the
config-directory
branch
from
c9fdd17 to
cb0861d
Compare
|
@stephencelis, I’ve just pushed changes that address your feedback. Please review them. I believe you should be the one to close the remaining comments. |
2 participants
The motivation behind this is to enable the ability to set a custom path for the configuration directory. The configuration path is generated when the
pfw logincommand is invoked, so the environment variable should already be present. I’ve tried to keep this simple and omitted adding an option for existing commands or the new command altogether.The order in which the configuration directory is resolved is as follows:
PFW_HOME→XDG_CONFIG_HOME/pfw→~/.pfwFixes #36.