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

Fix reference linking #4463

Merged
merged 5 commits into from
Jul 11, 2018
Merged

Fix reference linking #4463

merged 5 commits into from
Jul 11, 2018

Conversation

Kriskras99
Copy link
Contributor

@Kriskras99 Kriskras99 commented Jun 25, 2018

I somehow messed up with my previous pull request (#3633), and I only switched back to the main repository today.

Pull request #3633 (#3633) only included half of the files.
@OmgImAlexis OmgImAlexis added Bug Needs review Needs testing Requires testing to make sure it's working as intended labels Jun 26, 2018
@OmgImAlexis OmgImAlexis added this to the 0.2.7 milestone Jun 26, 2018
@@ -506,10 +506,20 @@ def symlink(cur_file_path, new_file_path):
self.log(u'Unable to link file {0} to {1}: {2!r}'.format
(cur_file_path, new_file_path, e), logger.ERROR)
raise EpisodePostProcessingFailedException('Unable to move and link the files to their new home')

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kriskras99 The build is failing due to the white spaces here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sharkykh thanks for the pointer

except reflink.ReflinkImpossibleError as msg:
if msg.args[0] == 'EOPNOTSUPP':
except (reflink.ReflinkImpossibleError, IOError) as msg:
if msg.args is not None and msg.args[0] == 'EOPNOTSUPP':
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer:

if isinstance(msg.args, tuple) and msg.args[0] == 'EOPNOTSUPP':

@@ -443,7 +443,7 @@ def reflink_file(src_file, dest_file):
}
)
copy_file(src_file, dest_file)
elif msg.args[0] == 'EXDEV':
elif msg.args is not None and msg.args[0] == 'EXDEV':
Copy link
Contributor

Choose a reason for hiding this comment

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

Same:

if isinstance(msg.args, tuple) and msg.args[0] == 'EXDEV':

@@ -434,7 +434,7 @@ def reflink_file(src_file, dest_file):
raise NotImplementedError()
reflink.reflink(src_file, dest_file)
except (reflink.ReflinkImpossibleError, IOError) as msg:
if msg.args is not None and msg.args[0] == 'EOPNOTSUPP':
if isinstance(msg.args, tuple) and msg.args[0] == 'EOPNOTSUPP':
Copy link
Contributor

@labrys labrys Jun 29, 2018

Choose a reason for hiding this comment

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

@sharkykh isinstance checks are not pythonic and should generally be avoided.

What do you intend to check here?
First of all both exceptions subclass from Exception which makes msg.args always a tuple.
Secondly the original check for is not None is equally flawed since it by default would be an empty tuple.

This should be if msg.args and msg.args[0] == 'EOPNOTSUPP':

Copy link
Contributor

Choose a reason for hiding this comment

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

It's used so much, how come it's not pythonic?
In any case, I agree the msg.args would always be a tuple with these exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sharkykh Python is a duck-typed language and generally speaking you are breaking one of the primary advantages of Python with isinstance checks.

It can also lead to error-prone assumptions. For example what if Python 2.7.16 comes out and changes msg.args to a list instead of a tuple? The code would be broken.

The important part of the code is to check the first element of msg.args. So as long as msg.args is subscriptable and you can get the first element with msg.args[0] the code should pass, regardless of msg.args's type.

There are some legitimate reasons where an isinstance is the correct way to do something (metaclasses and abstract base classes come to mind), but usually if you need to use them you already know there isn't a cleaner way to do it.

One of the only places outside of metaclasses that I will use isinstance checks regularly is to avoid iterating a string for a function that requires an iterable as an argument. For that I usually create a helper function though to make the intent clear:

def gen_items(it):
    """
    Generate items from an iterable.

    If the iterable is a string yield it without iterating it.

    :param it: An iterable of items.
    :returns: a generator that yields items sequentially
    """
    if isinstance(it, str):
        yield it
    else:
        for item in it:
            yield item

>>> gen_items('cake')
<generator object gen_items at 0x000001F834DEADE0>

>>>list('cake')
['c', 'a', 'k', 'e']

>>> list(gen_items('cake'))
['cake']

>>> list(gen_items(['a', 1, 'cake']))
['a', 1, 'cake']

>>> list(gen_items([]))
[]

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the thorough explanation.

@@ -443,7 +443,7 @@ def reflink_file(src_file, dest_file):
}
)
copy_file(src_file, dest_file)
elif msg.args is not None and msg.args[0] == 'EXDEV':
elif isinstance(msg.args, tuple) and msg.args[0] == 'EXDEV':
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@sharkykh
Copy link
Contributor

@Kriskras99
Now you need to merge develop into this branch (or rebase) for the checks to pass.
Other than that, LGTM.

Merge upstream to fix CI's
@medariox medariox merged commit f8fbd33 into pymedusa:develop Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Concluded Needs testing Requires testing to make sure it's working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants