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

Consolidate behavior by using filterfalse and always_iterable #3265

Merged
merged 2 commits into from
Mar 1, 2018

Conversation

jaraco
Copy link
Contributor

@jaraco 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
Copy link
Member

restarting the build due to network issues on ci

@coveralls
Copy link

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
Copy link
Member

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)

@nicoddemus
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Added a CHANGELOG entry. 👍

@RonnyPfannschmidt RonnyPfannschmidt merged commit 3b757b1 into master Mar 1, 2018
@RonnyPfannschmidt RonnyPfannschmidt deleted the feature/always-iterable-refactor branch March 1, 2018 07:52
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Mar 4, 2018
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants