Add support for XDG_* environment variables#375
Conversation
gaborbernat
left a comment
There was a problem hiding this comment.
Rather than duplicating all this code between Linux and Mac OS, can we somehow share it? Maybe they should have a common base class XDG?
|
I start thinking about add decorator. Sometthing like: def from_env_variabe(var_name):
def wrap_(func):
def func_(self):
if val := os.environ.get(var_name, "").strip():
return self._append_app_name_and_version(os.path.expanduser(val))
return func(self)
return func_
return wrap_and then: @property
@from_env_variabe("XDG_CONFIG_HOME")
def user_config_dir(self) -> str:
return self.user_data_dirIt reduces code duplication but may be unclear for maintenance. |
|
@gaborbernat Any opinion? I see two options. The decorator, or some dict/new properties to store default values. |
|
I don't like either 🤔 the decorator or the dict requries us to duplicate some code between repos. Perhaps we need to create a XDG base class mix-in, and inherit from both and let inheritance take care of it? |
|
By mixins, you understand: class XDGMixin(PlatformDirsABC):
@property
def user_cache_dir(self):
if env_var := os.environ.get('XDG_CACHE_HOME', '').strip():
return self._append_app_name_and_version(os.path.expanduser(env_var)
return super().user_cache_dir
...
class MacOSBase(PlatformDirsABC):
@property
def user_cache_dir(self):
return self._append_app_name_and_version(os.path.expanduser("~/Library/Caches"))
class MacOS(XDGMixin, MacOSBase):
passsIn my opinion, it is less readable than: class MacOSBase(PlatformDirsABC):
@env_property('XDG_CACHE_HOME')
def user_cache_dir(self):
return self._append_app_name_and_version(os.path.expanduser("~/Library/Caches"))That I did not suggest it earlier. Did I understand you correctly? If not, could you provide some code snippets? |
|
It's not only about readability, it's also about how we make sure that macos and unix aligns up, and we don't miss setting it in one but not in another. |
It could be done via tests. I correctly understand that I should follow the pattern from the previous post? What about docstrings? Should I implement in |
I don't think belongs to docstrings.
It would require us to remember to add these tests whenever we add new endpoints.
Not entirely sure, looking at the code I don't like mixin either; was just raising the problem, not sure yet what's the best solution. |
|
Maybe classical inheritance: class XDGBase(PlatformDirsABC):
@abstractmethod
def user_cache_dir_default():
pass
@property
def user_cache_dir(self):
if env_var := os.environ.get('XDG_CACHE_HOME', '').strip():
return self._append_app_name_and_version(os.path.expanduser(env_var))
return self.user_cache_dir_default()and class MacOS(XDGBase):
def user_cache_dir_default(self):
return self._append_app_name_and_version(os.path.expanduser("~/Library/Caches") |
|
Yeah 🤔 |
d3a64d2 to
91496db
Compare
91496db to
c600490
Compare
macOS users who set XDG environment variables (common with Homebrew and dotfile managers) had no way to override Apple's default directories. Meanwhile, Unix already handled XDG vars but the logic was baked into the Unix class, making it impossible to share with macOS. Extract XDG env var handling into an XDGMixin that both Unix and MacOS inherit from. Each platform provides defaults in a base class (_UnixDefaults, _MacOSDefaults), and the mixin checks XDG vars first, falling through to platform defaults via super(). This ensures both platforms stay in sync — adding a new XDG property to the mixin automatically applies to both.
c600490 to
a21c5e6
Compare
# References and relevant issues # Description The last `appdirs` release was May 11 2020. Then `platformdirs` take its place. It introduces many improvements like allow to overwrite dirs using `XDG_*` variables tox-dev/platformdirs#375 So I think we should switch to platformdirs
As decided in #374
#374 (comment)