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

empty_is_nil and strip options should cooperate. #148

Merged
merged 1 commit into from
Apr 8, 2020

Conversation

saverio-kantox
Copy link
Contributor

If option strip is given, a blank string will become empty. But empty_is_nil was already checked, so it is not converted to nil.

With this change, when both empty_is_nil and strip are given, an input like " " is correctly converted to nil.

@eugeneius
Copy link
Collaborator

Thanks for this! I agree that the options should work this way when applied together. 👍

The test suite should pass if you rebase to pick up 36018de.

What do you think of this implementation, which avoids duplicating the strip logic?

diff --git a/lib/mutations/string_filter.rb b/lib/mutations/string_filter.rb
index a947a6f..ce3fd5e 100644
--- a/lib/mutations/string_filter.rb
+++ b/lib/mutations/string_filter.rb
@@ -15,10 +15,6 @@ module Mutations
     }

     def filter(data)
-      if options[:empty_is_nil] && data == ""
-        data = nil
-      end
-
       # Handle nil case
       if data.nil?
         return [nil, nil] if options[:nils]
@@ -39,7 +35,9 @@ module Mutations

       # Now check if it's blank:
       if data == ""
-        if options[:empty]
+        if options[:empty_is_nil]
+          return filter(nil)
+        elsif options[:empty]
           return [data, nil]
         else
           return [data, :empty]

Would you mind also adding a test?

@saverio-kantox
Copy link
Contributor Author

I'll add the test.

Regarding the proposal, I had developed for internal use some input filters with recursive calls to #filter, but I find them very obscure.

When the recursive call is isolated, in a pull request, it is very visible. But then, out of context with all the surrounded method, the single recursive call in a 45-line method will clearly be overseen, and potentially a cause of bugs.

So I'd really prefer to be more explicit.

@saverio-kantox
Copy link
Contributor Author

But you are right that the check should be done in the same context as the other empty check.

@saverio-kantox saverio-kantox force-pushed the patch-1 branch 2 times, most recently from e4342e5 to e57d8b2 Compare April 8, 2020 09:07

Unverified

The committer email address is not verified.
If option `strip` is given, a blank string will become empty. But `empty_is_nil` was already checked, so it is not converted to nil.

With this change, when both `empty_is_nil` and `strip` are given, an input like `"     "` is correctly converted to nil.
@saverio-kantox
Copy link
Contributor Author

Travis is 💚

@saverio-kantox
Copy link
Contributor Author

And thanks for the suggestion

@eugeneius eugeneius merged commit a99d8ac into cypriss:master Apr 8, 2020
@eugeneius
Copy link
Collaborator

Thanks again @saverio-kantox! I just released 0.9.1, which includes this change.

@saverio-kantox saverio-kantox deleted the patch-1 branch April 14, 2020 06:30
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.

None yet

2 participants