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

Update matcher to take list of arguments; alias it as #strip_attributes #52

Merged
merged 2 commits into from
Mar 31, 2020
Merged

Update matcher to take list of arguments; alias it as #strip_attributes #52

merged 2 commits into from
Mar 31, 2020

Conversation

experimatt
Copy link
Contributor

@rmm5t here's a PR for the issue I created: #51

I tried to match your style as much as possible, but please let me know if you'd like me to make any changes.

Copy link
Owner

@rmm5t rmm5t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks great. Thank you. Just one small adjustment, please.

subject.send("#{@attribute}=", " string ")
subject.valid?
subject.send(@attribute) == "string" and collapse_spaces?(subject)
end.all?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[PLEASE CHANGE] no need to pass over the array twice:

        @attributes.all? do |attribute|
          @attribute = attribute
          subject.send("#{@attribute}=", " string ")
          subject.valid?
          subject.send(@attribute) == "string" and collapse_spaces?(subject)
        end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Updated, and I also removed the extra unnecessary splats (*)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like me to squash the commits into one, or is there an automatic way github can do that with the merge (I'm used to gitlab)?

@rmm5t rmm5t merged commit b750faf into rmm5t:master Mar 31, 2020
@rmm5t
Copy link
Owner

rmm5t commented Mar 31, 2020

Squashed and merged. Thank you! 🍻

@rmm5t
Copy link
Owner

rmm5t commented Mar 31, 2020

v1.10.0 released with these changes!
https://rubygems.org/gems/strip_attributes

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.

2 participants