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

Ensure fair error attribution of pipelined return codes #243

Merged
merged 7 commits into from Nov 13, 2015

Conversation

hashbrowncipher
Copy link
Contributor

Fixes #241 by making the (new) verify method on command objects responsible for determining whether to raise an exception.

@hashbrowncipher
Copy link
Contributor Author

Worth noting is that my penultimate commit modifies a test that would otherwise fail due to a behavioral change. I believe that fixing the test, as it was originally written, would require implementing the TODO I left on line 303 of base.py.

@henryiii
Copy link
Collaborator

This looks good to me. I'll let @tomerfiliba or @koreno check it over, then I'm okay to merge. I will need this merged before I submit my fix for #240.

@henryiii
Copy link
Collaborator

I'll go ahead and merge this, so I can add a few help strings and submit my patch.

henryiii added a commit that referenced this pull request Nov 13, 2015
Ensure fair error attribution of pipelined return codes
@henryiii henryiii merged commit 9cf65f8 into tomerfiliba:master Nov 13, 2015
@henryiii
Copy link
Collaborator

We'll need to fix that test before releasing 1.6.1, I believe, as only requiring the final error code seems more intuitive (and it's the way it used to work). I'm looking at that, feel free to do so too.

@henryiii
Copy link
Collaborator

I believe the issue really is that plumbum follows a non-standard return code policy. That is,
false | true has a return code of 0 in bash. Plumbum uses or to combine return code, making 0 special regardless of your requested return code.

I've set it so that 0 is expected for everything except the last command, which fixes the test that failed because it is the way plumbum has always worked. It just didn't attribute the non-zero command because it was a simple or, rather than the method of this patch.

If we can attach an expected return code to processes, that would be the ideal way to allow custom return code management. That should not be hard now that each command is properly tested.

henryiii added a commit that referenced this pull request Nov 13, 2015
…ng, things that worked before now still work
@henryiii
Copy link
Collaborator

Correction: I've now set it so that the old behavior is exactly restored (though with the correct attribution now in place). We can discuss changing it for 1.7.0, but probably not for v1.6.1. I think adding a way to set each command's return code is probably the way to go from here, though. Like:

ls['-a'].set_retcode([0,1]) & FG

Where .set_retcode sets a property and returns self. Then, if that property exists / is not some special value, that is used for verify instead. A little verbose, but don't see another better way ATM.

@henryiii henryiii added this to the v1.6.1 milestone Nov 13, 2015
@henryiii henryiii added the Bug label Nov 13, 2015
@hashbrowncipher
Copy link
Contributor Author

That's gorgeous, thanks @henryiii.

I think your solution involving set_retcode is a good one. I would recommend normative names like 'expect_code' or 'should_return' or (if you like brevity) 'returns'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants