Skip to content
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

Use a single standard for config/cache/state files #24

Closed
tobbez opened this issue Jul 10, 2016 · 14 comments
Closed

Use a single standard for config/cache/state files #24

tobbez opened this issue Jul 10, 2016 · 14 comments
Labels
Bug Generic bug: can be used together with more specific labels
Milestone

Comments

@tobbez
Copy link
Contributor

tobbez commented Jul 10, 2016

Not everything makes use of morituri.common.directory.Directory to decide where to read/write files, which leads to some files ending up in /.morituri/, and others in XDG directories (/.config/morituri, ~/.cache/morituri by default).

Everything should either follow XDG or the old standard.

# Uses of Directory.getConfig
morituri/common/config.py:        return directory.Directory().getConfig()
morituri/test/test_common_directory.py:        path = d.getConfig()

# Uses of Directory.getCache
morituri/common/cache.py:            self._path = d.getCache('table')
morituri/test/test_common_directory.py:        path = d.getCache()

# Uses of old style config directory
morituri/common/accurip.py:_CACHE_DIR = os.path.join(os.path.expanduser('~'), '.morituri', 'cache')
morituri/common/cache.py:        path = os.path.join(os.path.expanduser('~'), '.morituri', 'cache',
morituri/rip/main.py:    homepluginsdir = os.path.join(os.path.expanduser('~'),

It would also make sense to only allow one location for the config file, especially if we are to rename it (i.e. whipper.conf rather morituri.conf). Right now both ~/.config/morituri/morituri.conf and ~/.moriturirc are allowed.

@JoeLametta JoeLametta added the Bug Generic bug: can be used together with more specific labels label Jul 12, 2016
@JoeLametta JoeLametta added this to the 1.0 milestone Jul 12, 2016
@JoeLametta
Copy link
Collaborator

This is indeed inconsistent and quite annoying: something I really wanted to improve. As soon I have enough time I'll work on this one too.

@JoeLametta
Copy link
Collaborator

JoeLametta commented Jul 24, 2016

@tobbez Done: not yet merged into master (still in its own branch).
Let me know what you think about this (for more details, please read commits' description).

@JoeLametta
Copy link
Collaborator

JoeLametta commented Aug 29, 2016

@neitsab Sorry if I bother you again.
Before I proceed to close this issue and merge the development branch into master's one I wanted to let you know about this backward incompatible change (for more information, please read 1 & 2).

Maybe it would be a good idea to mention it in the pkgbuild install file.

@tobbez
Copy link
Contributor Author

tobbez commented Aug 30, 2016

It would be better to remove the fallback completely, since it would mean less code. It would also make support easier because it would make sure there's always only a single location were config/cache/state would be stored.

The workarounds for older versions of pyxdg should also be removed (the almost 4 years since 0.25 was released should be enough for it to be available almost everywhere).

The changes mentioned above would leave us with this diff for directory.py (applies against master, download here)

diff --git a/morituri/common/directory.py b/morituri/common/directory.py
index 47aac11..7af0df3 100644
--- a/morituri/common/directory.py
+++ b/morituri/common/directory.py
@@ -22,34 +22,21 @@

 import os

+from xdg import BaseDirectory
+
 from morituri.common import log


 class Directory(log.Loggable):
-
     def getConfig(self):
-        try:
-            from xdg import BaseDirectory
-            directory = BaseDirectory.save_config_path('morituri')
-            path = os.path.join(directory, 'morituri.conf')
-            self.info('Using XDG, configuration file is %s' % path)
-        except ImportError:
-            path = os.path.join(os.path.expanduser('~'), '.moriturirc')
-            self.info('Not using XDG, configuration file is %s' % path)
+        directory = BaseDirectory.save_config_path('whipper')
+        path = os.path.join(directory, 'whipper.conf')
+        self.info('Configuration file is %s' % path)
         return path

-
     def getCache(self, name=None):
-        try:
-            from xdg import BaseDirectory
-            path = BaseDirectory.save_cache_path('morituri')
-            self.info('Using XDG, cache directory is %s' % path)
-        except (ImportError, AttributeError):
-            # save_cache_path was added in pyxdg 0.25
-            path = os.path.join(os.path.expanduser('~'), '.morituri', 'cache')
-            if not os.path.exists(path):
-                os.makedirs(path)
-            self.info('Not using XDG, cache directory is %s' % path)
+        path = BaseDirectory.save_cache_path('whipper')
+        self.info('Cache directory is %s' % path)

         if name:
             path = os.path.join(path, name)
@@ -59,25 +46,12 @@ class Directory(log.Loggable):
         return path

     def getReadCaches(self, name=None):
-        paths = []
-
-        try:
-            from xdg import BaseDirectory
-            path = BaseDirectory.save_cache_path('morituri')
-            self.info('For XDG, read cache directory is %s' % path)
-            paths.append(path)
-        except (ImportError, AttributeError):
-            # save_cache_path was added in pyxdg 0.21
-            pass
-
-        path = os.path.join(os.path.expanduser('~'), '.morituri', 'cache')
-        if os.path.exists(path):
-            self.info('From before XDG, read cache directory is %s' % path)
-            paths.append(path)
+        path = BaseDirectory.save_cache_path('whipper')
+        self.info('Read cache directory is %s' % path)

         if name:
-            paths = [os.path.join(p, name) for p in paths]
-
-        return paths
-
+            path = os.path.join(path, name)
+            if not os.path.exists(path):
+                os.makedirs(path)

+        return [path]

which would leave us with this (header omitted):

import os

from xdg import BaseDirectory

from morituri.common import log


class Directory(log.Loggable):
    def getConfig(self):
        directory = BaseDirectory.save_config_path('whipper')
        path = os.path.join(directory, 'whipper.conf')
        self.info('Configuration file is %s' % path)
        return path

    def getCache(self, name=None):
        path = BaseDirectory.save_cache_path('whipper')
        self.info('Cache directory is %s' % path)

        if name:
            path = os.path.join(path, name)
            if not os.path.exists(path):
                os.makedirs(path)

        return path

    def getReadCaches(self, name=None):
        path = BaseDirectory.save_cache_path('whipper')
        self.info('Read cache directory is %s' % path)

        if name:
            path = os.path.join(path, name)
            if not os.path.exists(path):
                os.makedirs(path)

        return [path]

@JoeLametta
Copy link
Collaborator

@tobbez
Thanks for the feedback.
Updated it again, check it out and let me know what's your opinion about the changes (no differing paths, PyXDG isn't used anymore).

@tobbez
Copy link
Contributor Author

tobbez commented Sep 3, 2016

Why not unconditionally depend on PyXDG, instead of re-implementing parts of it?

PyXDG is tested, stable, small, available everywhere, so I see no reason to against just using it.

Apart from that, there's superfluous code left in getReadCaches for handling multiple cache paths. Since we can only ever have one in whipper (unlike morituri) it can be simplified (compare to the code at the end of my previous comment). The plan would be to also make it return a single path instead of a list and rename it to getReadCache, but that can be done later (since it requires modifying all code that calls the function as well).

@neitsab
Copy link

neitsab commented Sep 4, 2016

@JoeLametta No problem for adding a warning about backwards-incompatible changes in the install file. Could you tag this release so that we have a precise version to link against?

JoeLametta added a commit that referenced this issue Sep 5, 2016
Now whipper always follows XDG specifications. All changes were documented into the README file.
@JoeLametta
Copy link
Collaborator

@tobbez

PyXDG is simply not needed in this case.
I've simplified the getReadCaches function.

If we rewrite the getReadCaches function to just return a string why don't we drop it entirely leaving only getCache (as in both cases we would have to fix the calls to the function) ?

@neitsab

Thanks, will do once I've finished working on this branch.

@JoeLametta
Copy link
Collaborator

@tobbez

I've just dropped the getReadCaches function (getCache should suffice). I haven't tested the change and I'm also a bit sleepy but Travis CI says "build passing" so hopefully it's fine.
If you can, please review it and let me know what's your opinion about it.

Thanks

@JoeLametta JoeLametta mentioned this issue Oct 9, 2016
JoeLametta added a commit that referenced this issue Oct 9, 2016
* Address issue #24
* Remove getReadCaches() & replace it with getCache()

Now whipper always follows XDG specifications. All changes were documented into the README file.

The changes introduced here aren't backward compatible so, after updating whipper, you may need to configure it again (old config file are retained, though).
@JoeLametta
Copy link
Collaborator

@neitsab

No problem for adding a warning about backwards-incompatible changes in the install file. Could you tag this release so that we have a precise version to link against?

Done.

v0.2.4

@neitsab
Copy link

neitsab commented Oct 16, 2016

@JoeLametta Thanks for the tag and sorry for the late reply, I was AFK for a while.

Working on the update to the AUR package right now :)

@JoeLametta
Copy link
Collaborator

JoeLametta commented Oct 16, 2016

@JoeLametta Thanks for the tag and sorry for the late reply, I was AFK for a while.

Working on the update to the AUR package right now :)

No problem.
As you are updating the package please include the new dependencies (stated here, GStreamer is still needed).

PS: The version tagged introduced the non backward compatible change but also a regression which was then fixed with 8b5ce57.

@neitsab
Copy link

neitsab commented Oct 16, 2016

Sure thing!

This update is a bit complex (several user requests and changes to accommodate), should I open a new issue to discuss them with you if I have questions or do you have another preferred communication channel?

@JoeLametta
Copy link
Collaborator

This update is a bit complex (several user requests and changes to accommodate), should I open a new issue to discuss them with you if I have questions or do you have another preferred communication channel?

Creating an issue labeled as question is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Generic bug: can be used together with more specific labels
Projects
None yet
Development

No branches or pull requests

3 participants