Make this work in windows #16

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

This would error out if attempted on windows. Expand userdir works on
many OS's without the HOME environment assumption. 

I've set the default function as disabled in windows -
it currently dies with an error when doing an add due to not having
the os.symlink capability.

The not in is used for other OS's that don't support this (if appropriate).
Ideally, I'd like to get support for Windows using another method - but
for now this gets me unstuck

@dannystaple dannystaple Make this work in windows
This would error out if attempted on windows. Expand userdir works on 
many OS's without the HOME environment assumption. 

I've set the default function as disabled in windows -
it currently dies with an error when doing an add due to not having 
the os.symlink capability.

The not in is used for other OS's that don't support this (if appropriate).
Ideally, I'd like to get support for Windows using another method - but 
for now this gets me unstuck
a9b94c5

@dylanahsmith dylanahsmith commented on the diff Aug 10, 2013

scripts/shopify_api.py
@@ -126,7 +126,7 @@ def remove(cls, connection):
"""remove the config file for CONNECTION"""
filename = cls._get_config_filename(connection)
if os.path.exists(filename):
- if cls._is_default(connection):
+ if cls._supports_default() and cls._is_default(connection):
@dylanahsmith

dylanahsmith Aug 10, 2013

Member

It would make more sense if _is_default just returned false.

@dylanahsmith

dylanahsmith Aug 10, 2013

Member

Actually, it looks like it would already be doing that. Since _default_connection would return None if there is no default file.

@dylanahsmith dylanahsmith commented on the diff Aug 10, 2013

scripts/shopify_api.py
@@ -231,6 +234,10 @@ def _session_from_config(cls, config):
session.api_key = config.get("api_key")
session.token = config.get("password")
return session
+
+ @classmethod
+ def _supports_default(cls):
+ return os.name not in ['nt']
@dylanahsmith

dylanahsmith Aug 10, 2013

Member

Don't newer versions of windows support symbolic links, and couldn't there be other platforms that don't support symlinks. I think we should do feature detection rather than os detection. Is the function not defined on windows? If not, then we can detect the presence of the method. Does the method exist but raise an error? If so, then try to read or write the symlink and rescue the error.

@dylanahsmith dylanahsmith commented on the diff Aug 10, 2013

scripts/shopify_api.py
@@ -163,6 +163,9 @@ def show(cls, connection=None):
@usage("default [CONNECTION]")
def default(cls, connection=None):
"""show the default connection, or make CONNECTION the default"""
+ if not cls._supports_default():
+ print("Sorry - this OS doesn't yet support default")
@dylanahsmith

dylanahsmith Aug 10, 2013

Member

There is no need to print this message if no arguments are provided, since default wouldn't exist, and it would print "There is no default connection set".

Member

dylanahsmith commented Aug 10, 2013

Sorry for the delay in reviewing this.

Contributor

kevinhughes27 commented Feb 26, 2014

This PR has been open for a long time, I'm going to close it now. Feel free to re-open if you'd like and we can discuss and re-evaluate making some changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment