judy2k / stupid-python-tricks

Stupid Python tricks.
The Unlicense
145 stars 27 forks source link

Refactored/Simplified ish module #4

Closed snoack closed 9 years ago

snoack commented 9 years ago

As the codebase of the ish module is growing as it increases in popularity and adopters, I found that the code needs a refactoring. Therefore I submitted this PR which does following improvements to the code:

judy2k commented 9 years ago

This sounds great. I'll take a look when I'm back at my laptop. Thanks for contributing!

judy2k commented 9 years ago

Don't forget to add yourself to CONTRIBUTORS. txt!

snoack commented 9 years ago

Are you still at EuroPython? Maybe we can have a sprint for this project!

judy2k commented 9 years ago

I wish I was! I'm in Bilbao but not at the sprints :(

snoack commented 9 years ago

I created two more PRs that are based on this one. However, it turned out that when you create a PR based on a not yet merged branch, it will be created for your own fork instead for the upstream repo:

snoack/stupid-python-tricks/pull/1 snoack/stupid-python-tricks/pull/2

So I simply added those commits to this PR now. Sorry for the confusion.

snoack commented 9 years ago

Here comes another commit. It fixes a regression introduced by the initial commit in this PR. This was that the ValueError raised when the value isn't recognized, included the normalized instead the original string. I also adapted the tests to protect against that scenario.

snoack commented 9 years ago

I realized that stripping digits might not be a that good idea, as '1' for example would be normalized to '' (empty string), evaluating to False, then. Fixed that, only stripping whitespaces and punctuation now.

Also, I reconsidered the empty string scenario. With #5, I suggested to add '' (empty-string) to FALSE_STRINGS. However, since strings are stripped before looked-up, " " and even "?" (with the change mentioned above) would be recognized as False-ish after normalization. So instead, we should check for empty string before normalization. Or even simpler, only entering the code path with the special handling for strings if other evaluates to True. Did that with the changes above.

snoack commented 9 years ago

I just rebased this PR on top of #3, which I rebased on top of master before.