spl0k / supysonic

Supysonic is a Python implementation of the Subsonic server API.
https://supysonic.readthedocs.io
GNU Affero General Public License v3.0
264 stars 58 forks source link

Update config.py #207

Closed charmparticle closed 2 years ago

charmparticle commented 3 years ago

The way the config detection was written before, I could only get supysonic working by renaming my /etc/supysonic/supysonic.conf to /etc/supysonic, which doesn't make sense and I don't think that's what you meant to do. With this change, either /etc/supysonic.conf or /etc/supysonic/supysonic.conf will be detected and loaded.

codecov[bot] commented 3 years ago

Codecov Report

Merging #207 (c25f42b) into master (0b67aeb) will decrease coverage by 0.05%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
- Coverage   88.53%   88.47%   -0.06%     
==========================================
  Files          35       35              
  Lines        3358     3358              
==========================================
- Hits         2973     2971       -2     
- Misses        385      387       +2     
Impacted Files Coverage Δ
supysonic/config.py 95.74% <ø> (ø)
supysonic/covers.py 80.39% <0.00%> (-3.93%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0b67aeb...c25f42b. Read the comment docs.

spl0k commented 3 years ago

Hello, I hope you had a good holiday and wish you a happy new year!

Loading /etc/supysonic is actually what I wanted to do, see the first paragraph of the configuration doc. Why do you think it doesn't make sense? Several programs read files straight from /etc, what doesn't make sense in my opinion is having a directory to hold only a single file.

Your change will break for existing users who put the configuration in /etc/supysonic or a supysonic.conf file in the current working directory (when the web server or daemon are started) as you didn't keep the old paths. Furthermore, the order of the paths in the common_path array is important as it defines the order in which the files are loaded. Here /etc/supysonic/supysonic.conf will have the lowest priority and /etc/supysonic.conf the highest, this last one overriding any configuration made at user level. User-level configuration should always take precedence over system-level configuration.

baldurmen commented 2 years ago

FWIW, I think having /etc/supysonic is perfectly fine and I would have to muck around with the Debian package and add an inelegant post-install script if that were to change, as otherwise it would break people's installations :)

spl0k commented 2 years ago

I don't know why I kept this open. This PR won't be merged, for the reasons I explained above.

To answer @baldurmen concerns, if we were to replace existing configuration paths (which we have no reason to do now) there will be some announced deprecation period first or, even better if possible, an automatic migration path. Breaking existing installations is wrong if it doesn't involve releasing a new major version.