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

Linux fixes #17

Closed
wants to merge 11 commits into from
Closed

Linux fixes #17

wants to merge 11 commits into from

Conversation

eddyp
Copy link
Contributor

@eddyp eddyp commented Feb 20, 2013

Fixes Issue #16
Bumps version to 1.3.0
Removes gratuitous case mangling on the case-sensitive Linux, since that's not wise
Fixes the uterly wrong behaviour in site_data_dir, return result based on XDG_DATA_DIRS and make room for respecting the standard which specifies XDG_DATA_DIRS is a multiple-value variable
Add *_config_dir which are distinct on nix-es, according to XDG specs; on Windows and Mac return the corresponding *_data_dir (Fixes Issue #6)

@simon-weber
Copy link

Is there a reason that these fixes are not being merged? Is this project abandoned?

If that's the case, are you planning on maintaining your fork, @eddyp?

@srid
Copy link
Contributor

srid commented Mar 14, 2013

we have just been busy with stackato work. i will take a look at the pull requests this weekend.

@ghost ghost assigned srid Mar 14, 2013
@simon-weber
Copy link

Great; thanks for checking it out, @srid.

@eddyp
Copy link
Contributor Author

eddyp commented Mar 15, 2013

Simon, thanks for your interest in my changes and the heads-up for the maintainers. I suspect now that there won't be a need for me to maintain a fork :-)

@srid : please note that on my master branch I have also merged all of @joeyespo 's pull requests which have fixes for issues #14, #11 (pull request #13) and #12. This should make your merge related work easier and reduced to a simple review of the code.

@eddyp
Copy link
Contributor Author

eddyp commented Mar 15, 2013

@srid: BTW, I think appdirs should also have some supplemental APIs for user and system config information dirs which are treated differently on *nix, and I think this is the reason why you thought there was some confusion in the spec. The reason for the confusion was probably because you tried to fit two types of different information in the single hole of 'data' information, kind of like trying to fit a square peg into a round hole.

@srid
Copy link
Contributor

srid commented Mar 18, 2013

@eddyp +1 on the XDG and the case-sensitiveness changes. would you modify CHANGES.rst accordingly?

/cc @trentm

@eddyp
Copy link
Contributor Author

eddyp commented Mar 18, 2013

I can do it, no problems. But it's not clear to me on which branch I should focus.

Also, do you agree with me there should be user_config_dir and site_config_dir or something of that sort?

@eddyp eddyp closed this Mar 18, 2013
@eddyp
Copy link
Contributor Author

eddyp commented Mar 18, 2013

@srid: I added the *_config_dir stuff, too.
Please note I still think the master branch is an easier merge (and that's where I did the tests of the fixes).

(Sorry for the accidental close)

@eddyp eddyp reopened this Mar 18, 2013
@srid
Copy link
Contributor

srid commented Mar 19, 2013

it's not clear to me on which branch I should focus.

@eddyp i plan to merge your 'master' branch and close the related pull requests. but before i do that, i need to verify the changes using tests. new features will need new tests as well. (anyone want to volunteer for adding travis-ci support?)

do you agree with me there should be user_config_dir and site_config_dir or something of that sort?

yup, actually having those functions becomes important in light of the XDG change.

@eddyp
Copy link
Contributor Author

eddyp commented Mar 19, 2013

@srid, I am waiting for @mcepl 's input on the multi-platform idea.

(Sorry for the spam, I am writing from my phone)

@eddyp
Copy link
Contributor Author

eddyp commented Mar 19, 2013

@srid: the *_config_dir changes are already done. Could you, please, check them?

The update in the Readme are mostly copy + paste.

I think some manual tests should be enough for now, until the test frame matures and the details are ironed out. Talking about formal tests at this stage is probably premature.

@mcepl
Copy link
Contributor

mcepl commented Mar 19, 2013

@eddyp sorry, what's the question?

Couple of thoughts though:

BTW, I would emphasize the word ‘lightweight’ … that is one more reason for me to make the library free of any external dependencies.

@eddyp
Copy link
Contributor Author

eddyp commented Mar 19, 2013

@mcepl: see issue #19 for the question. Is basically allowing one to test correct values are returned for all platforms while running the tests on any given OS (supported or not).

Regarding the rebase, I just checked locally, merging the current master into my master is trivial, but the CHANGES.rst file was not updated after the cleaned up test frame, not was the version information bumped, so I am waiting for those changes and some proof of concept tests to test the actual values returned.

@mcepl mcepl mentioned this pull request Mar 19, 2013
@mcepl
Copy link
Contributor

mcepl commented Mar 19, 2013

Concerning CHANGES.rst, couldn’t we get rid of it completely? git log --pretty=medium (with GOOD commit messages) should be good for everybody, shouldn’t it?

@eddyp
Copy link
Contributor Author

eddyp commented Mar 19, 2013

AIUI, that file is for end users, and due to people like myself preferring atomic/small commit chunks, such a log would be useless for end users and would have to be filtered.

Still, dropping the file is @srid's decision.

@@ -216,11 +320,13 @@ def user_log_dir(appname, appauthor=None, version=None, opinion=True):

class AppDirs(object):
"""Convenience wrapper for getting application dirs."""
def __init__(self, appname, appauthor, version=None, roaming=False):
def __init__(self, appname, appauthor=None, version=None,
roaming=False, returnlist=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

having to pass the return data type of a bunch of methods in the class initializer seems strange to me. how about providing alternative functions for returning multiple values? for example: site_config_dir vs site_config_dirs (note the plural form)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type of the data doesn't change at all. It is always a string. The difference is that is a string containing mutiple paths separated by os.sep, as the XDG standard requires.

Do you think another name like 'multivalue' or 'firstonly' would have been a better name?
(note that with 'firstonly' the logic must be inverted)

Copy link
Contributor

Choose a reason for hiding this comment

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

i see. returnlist returns multiple paths as a single string separated by os.pathsep. is there a reason why it is not returning an actual list of paths? i'm not sure what the use case of returning multiple paths is; i'm guessing that users want to search for files -- say themes/plugins in /usr/share, ~/.local/share, etc -- in each and every paths, in which case they would probably use it as ,

for path in user_config_dir(returnlist=True).split(os.path)

as the XDG standard requires

keep in mind that appdirs API -- function arguments, return value format/type, etc. -- should ideally not be tied in to platform-specific concerns.

Do you think another name like 'multivalue' or 'firstonly' would have been a better name?

was it that use case (my guess above) that you had in mind when implementing the returnlist feature? if so, i find the following to be more straightforward:

for path in user_config_dir(returnlist=True):

even better, as the function name is currently in singular form:

# note the _list prefix, which is probably better than adding a single "s" to the function name
for path in user_config_dir_list():

explicit is better than implicit. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a reason why it is not returning an actual list of paths?

I did not want to have the function return sometimes a string and other times a list of strings. I don't consider it a sane practice to have a function return an unknown type, it can break applications very badly. Keeping it a string means the same type, string, is the return type.

i'm not sure what the use case of returning multiple paths is; i'm guessing that users want to search for file

Yes, this is correct. Please note that the XDG standard states there are site specific data dirs $XDG_DATA_DIRS and site specific config dirs $XDG_CONFIG_DIRS, but there is only a user specific data dir $XDG_DATA_HOME and a user specific config dir $XDG_CONFIG_HOME.

appdirs API -- should ideally not be tied in to platform-specific concerns.

That's why I tried to keep it backwards compatible, and for *_config_dir APIs on Windows and Mac I returned the *_data_dir information.

Even if we opt for site_*_dir_list APIs for multivalues, the API will be useful for *nix and the other two will only wrap the site_data_dir and we'll effectively have 4 APIs which essentially are identical due to wrapping and due to lack of dir lists. Please take this into account, too.

I think I prefer the site_config_dir_list variant as a disambiguation, although the site_config_dir API will probably remain unused on *nix and on Windows and Mac site_config_dir, site_data_dir, site_config_dir_litst and site_data_dir_list are essentially identical.

I am saying on *nix it will remain unused because, usually, applications DO want to search for files in /usr/local/share, then, if not found, in /usr/share. The order is significant and the mechanism is a fallback type of mechanism.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, thinking about it - adding a couple of extra functions that won't be used on win/mac doesn't appeal to me. actually, it looks like returning "multipath" (paths separated by os.pathsep) is already a good workaround. this is what you already have.

though, can we rename returnlist to multiple? as in:

site_data_dir(multiple=True) => "/usr/local/share:/usr/share"

mostly because it is not really returning a list date type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the rename.

How about multivalue or mutipath? mutiple has mathematical co-notations, which might be confusing (at least it does for me and it could be confusing).

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for multipath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I do the changes on my master and we'll ignore linux-fixes from now on?
I can create a 'next' branch based on my master with the current official master merged in, it you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Tue, Mar 19, 2013 at 1:55 PM, eddyp notifications@github.com wrote:

Should I do the changes on my master and we'll ignore linux-fixes from now
on?
I can create a 'next' branch based on my master with the current official
master merged in, it you want.

sure, 'next' works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

next added

https://github.com/eddyp/appdirs/tree/next

(sorry, I can't find the correct syntax to link correctly the branch)

@srid
Copy link
Contributor

srid commented Mar 19, 2013

@srid: the *_config_dir changes are already done. Could you, please, check them?

done ... and commented on the changes as well.

AIUI, that file is for end users, and due to people like myself preferring atomic/small commit chunks, such a log would be useless for end users and would have to be filtered.

correct.

@eddyp
Copy link
Contributor Author

eddyp commented Mar 21, 2013

@srid: do you have any other comments regarding the code ?
The current status is:

I have replied to your comment regarding the startswith/in code and opened a new issue to deal with all such occurrences.

The returnlist -> multipath change is implemented.

The current ActiveState/master is merged in my eddyp/next branch.

So I am waiting for your input or the actual merge.

@srid
Copy link
Contributor

srid commented Mar 21, 2013

@eddyp - i'm looking into your branch in origin/eddy, so far:

on linux

-- app dirs (without optional 'version')
user_data_dir: /home/sridharr/.local/share/MyApp/1.0
site_data_dir: /usr/share/xubuntu/MyApp/1.0
user_config_dir: /home/sridharr/.config/MyApp/1.0
site_config_dir: /usr/share/xubuntu/MyApp/1.0
user_cache_dir: /home/sridharr/.cache/MyApp/1.0
user_log_dir: /home/sridharr/.cache/MyApp/1.0/log

-- app dirs (with optional 'version')
user_data_dir: /home/sridharr/.local/share/MyApp
site_data_dir: /usr/share/xubuntu/MyApp
user_config_dir: /home/sridharr/.config/MyApp
site_config_dir: /usr/share/xubuntu/MyApp
user_cache_dir: /home/sridharr/.cache/MyApp
user_log_dir: /home/sridharr/.cache/MyApp/log

on mac

-- app dirs (without optional 'version')
user_data_dir: /Users/sridharr/Library/Application Support/MyApp/1.0
site_data_dir: /Library/Application Support/MyApp/1.0
user_config_dir: /Users/sridharr/Library/Application Support/MyApp/1.0
site_config_dir: /Library/Application Support/MyApp/1.0
user_cache_dir: /Users/sridharr/Library/Caches/MyApp/1.0
user_log_dir: /Users/sridharr/Library/Logs/MyApp/1.0

-- app dirs (with optional 'version')
user_data_dir: /Users/sridharr/Library/Application Support/MyApp
site_data_dir: /Library/Application Support/MyApp
user_config_dir: /Users/sridharr/Library/Application Support/MyApp
site_config_dir: /Library/Application Support/MyApp
user_cache_dir: /Users/sridharr/Library/Caches/MyApp
user_log_dir: /Users/sridharr/Library/Logs/MyApp

do you have a windows machine to check the output?

@srid
Copy link
Contributor

srid commented Mar 21, 2013

n/m .. i've merged it to master now. but we will need proper tests (issue #19) before cutting a release to pypi. closing this issue now, thanks for the work.

@srid srid closed this Mar 21, 2013
@eddyp eddyp deleted the linux-fixes branch March 21, 2013 21:23
@eddyp
Copy link
Contributor Author

eddyp commented Mar 21, 2013

I removed my linux-fixes branch since, in the end, 'next' was merged instead.

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.

None yet

4 participants