Add `normalize` matcher by stephannv · Pull Request #1558 · thoughtbot/shoulda-matchers

stephannv · 2023-05-22T12:30:33Z

Why

Rails 7.1 adds normalizes API.

I think would be nice to shoulda-matchers to have a matchers to easily test the new normalizes API, similar to allow_value/allow_values matchers. I'm using on my project and this:

it "normalizes email" do
  user = User.new(email: " \n myEmail@here.com \n\n")
  user.normalize_attribute(:email)
  expect(user.email).to eq "myemail@here.com"
end

Will become this:

it { is_expected.to normalize(:email).from(" \n myEmail@here.com \n\n").to("myemail@here.com") }

What do you think about? This PR makes sense?

Some considerations:

I'm not sure about the API normalize(attr), from(value) and to(value), I'm open to change.

normalizes accepts apply_to_nil option, but I think this should be tested in its own spec, like

shoulda normalize(:name).from(nil).to("Untitled")

Another option would be:

shoulda normalize(:name).from(" jane doe ").to("Jane Doe").normalizing_nil_to("Untitled")

I had to add Rails 7.1 to Appraisals to be able to test. I'm not sure how to restrict this tests to run only in rails_7_1. Other questions: Do we need to flag this matcher to run only in Rails 7.1 projects? Do we need to add this info to the docs?
I think this is my first PR to shoulda-matchers, so, feedback are welcome.
This PR is a work in progress, I opened it to receive feedbacks, so some non-related tests can be broken, or rubocop can be failing.

mcmire

Hey @stephannv, I think this is a great idea! I would probably suggest that we extract adding Rails 7.1 to another PR as it may cause CI failures. As for whether we should handle the apply_to_nil option by adding another method or not, I think we should keep it simple and stick to from and to like you've done here.

One thing about this is that we might want to implement a negative version of the matcher. Every matcher can be executed via should_not, and that may match or may not match, just as with should. We might want to have another failure message in that case.

Other than that this looks great!

stephannv · 2023-05-23T20:59:15Z

Done @mcmire . Can you take a look again?

I tried to create another PR focused on adding Rails 7.1 to pipeline but I faced some issues, I will give another try later.

Sub-Xaero · 2023-09-06T17:43:46Z

Any updates on this PR?
Happy to help out if more needs doing to take it over the line.

stephannv · 2023-09-06T17:48:11Z

Any updates on this PR? Happy to help out if more needs doing to take it over the line.

@Sub-Xaero I was waiting 7.1 release to update this PR, but I think I don't need to wait.

stephannv · 2023-09-06T18:03:30Z

@Sub-Xaero I remember now, this PR is waiting shoulda-matchers to support Rails 7.1. If you want to help, I think you can open a PR with necessary changes to prepare shoulda-matchers for Rails 7.1

stephannv · 2023-11-17T19:41:10Z

Hi @matsales28 , are you the new code owner for this PR? I rebased it with main, I think it's ready for review now. Your feedback will be very much appreciated.

matsales28 · 2023-11-24T13:01:26Z

Hi @matsales28 , are you the new code owner for this PR? I rebased it with main, I think it's ready for review now. Your feedback will be very much appreciated.

Yes, I'll look at this PR next week. I'm pretty busy at the moment with all the holidays.

matsales28

Overall it looks good to me. Left some small suggestions but once adjusted we're ready to merge. One thing that could make it even better is when we're dealing with several attributes, it would be awesome if we could check all of them and then provide an error message listing all the attributes that didn't pass the normalization. Right now, the way it's set up, only the first one that doesn't meet the criteria shows up in the error message. We can make this on an next PR.

stephannv · 2023-12-02T13:37:07Z

Overall it looks good to me. Left some small suggestions but once adjusted we're ready to merge. One thing that could make it even better is when we're dealing with several attributes, it would be awesome if we could check all of them and then provide an error message listing all the attributes that didn't pass the normalization. Right now, the way it's set up, only the first one that doesn't meet the criteria shows up in the error message. We can make this on an next PR.

Yeah @matsales28 , I think it would be a great improvement.

Thanks for the feedback, I think I addressed everything. Let me know if I'm missing something.

stephannv · 2023-12-09T19:34:50Z

Sorry for that @matsales28 , I think it's working now.

stephannv requested review from guialbuk, mcmire and vsppedro as code owners May 22, 2023 12:30

mcmire reviewed May 22, 2023

View reviewed changes

stephannv force-pushed the feat/normalize_matcher branch from c10b031 to 4e3f6b9 Compare May 23, 2023 20:34

stephannv force-pushed the feat/normalize_matcher branch from 4e3f6b9 to b824485 Compare November 17, 2023 18:58

stephannv requested a review from matsales28 as a code owner November 17, 2023 18:58

stephannv marked this pull request as draft November 17, 2023 18:58

stephannv force-pushed the feat/normalize_matcher branch 2 times, most recently from 1c6bacb to 0188a17 Compare November 17, 2023 19:15

stephannv changed the title ~~WIP: Add normalize matcher~~ Add normalize matcher Nov 17, 2023

stephannv marked this pull request as ready for review November 17, 2023 19:33

matsales28 reviewed Dec 1, 2023

View reviewed changes

Comment thread spec/unit/shoulda/matchers/active_record/normalize_matcher_spec.rb Outdated

Comment thread lib/shoulda/matchers/active_record/normalize_matcher.rb

stephannv force-pushed the feat/normalize_matcher branch from 0188a17 to 132c3c6 Compare December 2, 2023 13:32

stephannv requested a review from matsales28 December 2, 2023 13:37

matsales28 requested changes Dec 8, 2023

View reviewed changes

stephannv force-pushed the feat/normalize_matcher branch from 132c3c6 to 3e35b4c Compare December 9, 2023 19:12

Add normalize matcher

8169b29

stephannv force-pushed the feat/normalize_matcher branch from 3e35b4c to 8169b29 Compare December 9, 2023 19:14

stephannv requested a review from matsales28 December 9, 2023 19:35

matsales28 approved these changes Dec 11, 2023

View reviewed changes

matsales28 merged commit baabf89 into thoughtbot:main Dec 12, 2023

vsppedro mentioned this pull request Dec 22, 2023

[skip ci] Bring CHANGELOG up to date #1596

Merged

Uh oh!

Conversation

stephannv commented May 22, 2023

Why

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

stephannv commented May 23, 2023

Uh oh!

Sub-Xaero commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephannv commented Sep 6, 2023

Uh oh!

stephannv commented Sep 6, 2023

Uh oh!

stephannv commented Nov 17, 2023

Uh oh!

matsales28 commented Nov 24, 2023

Uh oh!

matsales28 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stephannv commented Dec 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephannv commented Dec 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sub-Xaero commented Sep 6, 2023 •

edited

Loading