Skip to content

Conversation

iKevinY
Copy link
Contributor

@iKevinY iKevinY commented Apr 6, 2015

This PR refactors __init__.py to use zip() for positional argument compatibility, and also fixes some miscellaneous capitalization/spelling errors.

  • The list returned by zip() is automatically truncated to the length of the shortest argument list, which greatly simplifies the code that loops through the list of positional arguments.
  • The expression if len(args): relies on the truthiness of len(args); using if args: does not change the behaviour of the conditional, but is easier to understand at a glance (since what's really being checked is whether or not there are any arguments).

iKevinY added 2 commits April 6, 2015 00:25
The list returned by zip() is automatically truncated to the length of
the shortest argument list, which greatly simplifies the code that loops
through the list of positional arguments.

The expression `if len(args):` relies on the truthiness of `len(args)`;
using `if args:` does not change the behaviour of the conditional, but
is easier to understand at a glance (the length of args is irrelevant).
@coveralls
Copy link

Coverage Status

Coverage increased (+0.14%) to 93.07% when pulling 79bf700 on iKevinY:minor-refactoring into cf7234d on waylan:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.14%) to 93.07% when pulling 79bf700 on iKevinY:minor-refactoring into cf7234d on waylan:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.14%) to 93.07% when pulling 79bf700 on iKevinY:minor-refactoring into cf7234d on waylan:master.

@mitya57
Copy link
Collaborator

mitya57 commented Apr 6, 2015

+1 from me.

@waylan
Copy link
Member

waylan commented Apr 6, 2015

Sorry but I'm having a little trouble understanding why this pull request was made. I'm sure the change is a good one -- if we were keeping that code, but the change even affected the code which raises the DeprecationWarning which indicates that this code will all be removed in the next release. If the code will all be deleted in the next release, then why make this change at all?

In fact, while working on the next release, I have already deleted this code. So it is unclear what value making this change would serve.

@iKevinY
Copy link
Contributor Author

iKevinY commented Apr 6, 2015

Ah, it didn't occur to me that this code was being removed in the next release; I agree that this change is unnecessary. Would it be alright to repurpose this PR into one that just contains iKevinY@40c968e?

@waylan
Copy link
Member

waylan commented Apr 6, 2015

Yes, that would be great. Thanks.

@iKevinY
Copy link
Contributor Author

iKevinY commented Apr 6, 2015

I found it irritating that the branch name was still going to be minor-refactoring despite not containing any refactoring, so I cherry-picked the commit from a new branch and opened #404.

@iKevinY iKevinY closed this Apr 6, 2015
@iKevinY iKevinY deleted the minor-refactoring branch April 6, 2015 23:17
@mitya57
Copy link
Collaborator

mitya57 commented Apr 13, 2015

If the code will all be deleted in the next release, then why make this change at all?

@waylan, do you mean that there will be no more releases from this repository? If yes, maybe it will make sense to stop accepting pull requests here and mention in the README that the code has been moved?

@waylan
Copy link
Member

waylan commented Apr 13, 2015

This repo is in "maintenance mode" and may receive future minor bug-fix releases. So far all accepted changes have fallen under that category. However you make a point, I probably should make a note of that somewhere.

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.

4 participants