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

Consolidate behavior by using filterfalse and always_iterable #3265

Merged
merged 2 commits into from Mar 1, 2018

Conversation

Projects
None yet
4 participants
@jaraco
Contributor

jaraco commented Feb 27, 2018

As I was searching through the code for a completely different purpose, I stumbled across this pattern, which I've seen many times before:

if variable is a singleton -> do one thing
otherwise if it's an iterable -> do the thing across each item

This common pattern is the reason I created always_iterable, as it allows looping over the items even if there's just one.

Using this technique, along with filterfalse, the code is more concise, less nested, and in my opinion, more directly conveys the intention. One slightly awkward aspect is that the for loop is abruptly terminated if it fails the isclass test, but that was the behavior before as well.

The main reservation I have about this change is that it adds a new dependency, and I'm unsure how important it is to avoid new dependencies. Given that more_itertools implements the recipes from itertools, I find that it tends to have broad appeal and would likely find beneficial use throughout the package (though I haven't surveyed it).

Thoughts?

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Feb 27, 2018

restarting the build due to network issues on ci

@coveralls

This comment has been minimized.

coveralls commented Feb 27, 2018

Coverage Status

Coverage increased (+0.01%) to 92.552% when pulling 14a9b1e on feature/always-iterable-refactor into 44fa5a7 on master.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Feb 27, 2018

ci is correct - -

since the project has various breaking changes over time, i wonder if its a good idea to make it a dependency tho, i'd like to discuss that a bit since i wonder if users would be afflicted by conflicts
but im generally positive about it (i think it can help to make more code more simple in pytest)

@RonnyPfannschmidt RonnyPfannschmidt requested a review from nicoddemus Feb 27, 2018

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Feb 27, 2018

I looked over more-itertools and it seems nice at first glance.

since the project has various breaking changes over time

You mean more-itertools? Which breakages do you mean?

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Feb 28, 2018

it went from 2.x to 4.x in 2017 - as far as i can tell the breaking changes where done for good reasons

@jaraco

This comment has been minimized.

Contributor

jaraco commented Feb 28, 2018

From my perspective, it’s always a risk that a dependency will introduce breaking changes, and rigorous use of Semver to communicate those changes helps mitigate that risk. If one wishes to accept this change but be conservative about possible backward incompatibilities, one may pin an upper bound on 5.0.

I wouldn’t recommend that though. In my experience, the backward incompatible changes have been small and ignorable for my projects. Much of the library is very stable (especially recipes). I’d recommend only blocking new major releases only if there is reason to think that release will affect the usage of that lib in this project.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Feb 28, 2018

Added a CHANGELOG entry. 👍

@RonnyPfannschmidt RonnyPfannschmidt merged commit 3b757b1 into master Mar 1, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@RonnyPfannschmidt RonnyPfannschmidt deleted the feature/always-iterable-refactor branch Mar 1, 2018

nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Mar 4, 2018

Reintroduce more_itertools dependency
Reintroduce more_itertools that was added in pytest-dev#3265, now in the features branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment