Skip to content

Conversation

Czaki
Copy link

@Czaki Czaki commented Aug 30, 2025

As decided in #374

#374 (comment)

@Czaki Czaki requested review from gaborbernat and ofek as code owners August 30, 2025 18:33
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Czaki
Copy link
Author

Czaki commented Aug 31, 2025

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_dir

It reduces code duplication but may be unclear for maintenance.

@Czaki
Copy link
Author

Czaki commented Sep 2, 2025

@gaborbernat Any opinion?

I see two options.

The decorator, or some dict/new properties to store default values.

@gaborbernat
Copy link
Member

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?

@Czaki
Copy link
Author

Czaki commented Sep 2, 2025

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):
    passs

In 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?

@gaborbernat
Copy link
Member

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.

@Czaki
Copy link
Author

Czaki commented Sep 2, 2025

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 *Base information about environment variables introduced by Mixin?

@gaborbernat
Copy link
Member

What about docstrings? Should I implement in *Base information about environment variables introduced by Mixin?

I don't think belongs to docstrings.

It could be done via tests.

It would require us to remember to add these tests whenever we add new endpoints.

I correctly understand that I should follow the pattern from the previous post?

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.

@Czaki
Copy link
Author

Czaki commented Sep 2, 2025

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")

@gaborbernat
Copy link
Member

Yeah 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants