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

Added absence validator for active model. #7155

Closed
wants to merge 14 commits into from

Conversation

robotex82
Copy link
Contributor

Please accept my pull request for an absence validator.

See following discussion: https://groups.google.com/forum/#!topic/rubyonrails-core/KS_Olcnlw8w

I included passing tests and docs (Copied and adapted from the presence validator).

Thanks!

Saludos,

Roberto

@paneq
Copy link
Contributor

paneq commented Jul 25, 2012

+1

@dolzenko
Copy link
Contributor

+1 Can confirm that needed this in almost every project with relatively complex models

@robotex82
Copy link
Contributor Author

I use it on trees, where i need attributes to be empty on roots. But I think, there should be better examples.

Anyone?

@chancancode
Copy link
Member

With the strict option set to true, this could be used to verify the integrity of your models... so I'm +1 on this.

@kendagriff
Copy link

+1 – Require first and last names for users older than 13, but prevent update for younger than 13.

end

def test_validates_acceptance_of_with_custom_error_using_quotes
Person.validates_absence_of :karma, :message => "This string contains 'single' and \"double\" quotes"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use 1.9 hash syntax. Thanks.

@frodsan
Copy link
Contributor

frodsan commented Oct 27, 2012

I'm 👍 on this.


module ActiveModel

# == Active Model Absence Validator
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move this title to class AbsenceValidator?

@frodsan
Copy link
Contributor

frodsan commented Oct 27, 2012

@robotex82 Could you add a CHANGELOG entry please? Thanks!

Topic.validates_absence_of(:title, :content)

t = Topic.new

Choose a reason for hiding this comment

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

Remove blank line.

@carlosantoniodasilva
Copy link
Member

I wonder if we should use present instead of not_blank, thoughts?

@robotex82 thanks!

@robotex82
Copy link
Contributor Author

Thank you guys!

Sorry, I have been busy all the time, but i'll have a look at your suggestions asap. Hopefully before next weekend.

Do you see any chance to have this merged into master for v4.0?

@carlosantoniodasilva
Copy link
Member

@robotex82 I believe so :)

@robotex82
Copy link
Contributor Author

I updated my local copy and changed add_on_not_blank to add_on_present.

I had some trouble with updating and rebasing - I admit, I lack experience on this topic - but I hope I didn't mess up.

Could someone please check my updated pull request?

What do I have to do next?

@carlosantoniodasilva
Copy link
Member

@robotex82 thanks, but apparently you have some commits that shouldn't be there, can you try a new rebase, so that we can see only your PR diff?

@robotex82
Copy link
Contributor Author

I did:

git rebase upstream/master
git push --force

is that correct?

Excuse my noobish questions! I googled for the correct process of updating forks, rebasing, etc. but it's still not really clear to me.


module ActiveModel
module Validations
# == Active Model Absence Validator

Choose a reason for hiding this comment

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

You can remove this comment.

@carlosantoniodasilva
Copy link
Member

@robotex82 it's fine now, thanks. I added some comments, and we'll need a two changelog entries in Active Model, one for Errors#add_on_present and other for the validation itself, can you please add them, and squash your commits? Thanks.

@robotex82
Copy link
Contributor Author

Should I append or prepend to the changelog?

@robotex82
Copy link
Contributor Author

Hope it's ok now.....

@carlosantoniodasilva
Copy link
Member

I added some minor comments, can you take a look at them before we merge? Also, what do you think about adding an example for the changelog entries, similar to the ones you added to the docs? (just look at other changelog examples to see how to format them).

Other than that, just squash your commits and we're good to go :). Thanks!

@robotex82
Copy link
Contributor Author

Well, I added the example to the changelog. But again I'm stuck with git :(

I have a commit history like that:

1a9f792 Updated changelog
072b977 Add `ActiveModel::Validations::AbsenceValidator`, a validator to check the  absence of attributes. Add `ActiveModel::Errors#add_on_present` method. Adds error messages to presen
c35fa0b Removed blank lines Changed add_on_not_blank to add_on_present
02e8148 Added absence validator for active model.
c233b2f Fix guides home links and maintain compatibility with small screens

So I thought I google how to squash commits. I tried some things but I keep getting errors or the commits don't go away.

So, please, could some of you experieced people, tell me how I package things up?

Thanks, and sorry again...squashing, rabaseing, etc. is new stuff to me.

@carlosantoniodasilva: I couldn't find your comments, where are they?

@carlosantoniodasilva
Copy link
Member

@robotex82 just look at the pull request Files Changed tab with the diff, and you'll see the comments (just ignore the "Remove blank line." ones).

Now about the rebase, Jon has just given a quick explanation here, but if you still have doubts, please let me know. Thanks!

@steveklabnik
Copy link
Member

@carlosantoniodasilva
Copy link
Member

@steveklabnik Ah true, I forgot that, awesome thanks ❤️

@steveklabnik
Copy link
Member

You didn't forget that at all; I saw this comment, was like "man, I really need to put this in a place where I can find it again," posted it, and then linked it here. ;)

@carlosantoniodasilva
Copy link
Member

Haha sorry, I thought I had seen that among your other contribution related posts. Awesome anyway, you were fast on that 😄

This is already handled by #find, it's a duplicate check, since
find_with_ids is not called from anywhere else.
Closes rails#8121

The `plugin new` generator always adds the dummy app rake tasks,
when a dummy app was created.
…bjects with unstable class names and instance variables.

Refactor FlashHash to take values for its ivars in the constructor, to pretty up FlashHash.from_session_value.

Remove stale comment on FlashHash: it is no longer Marshaled in the session so we can change its implementation.

Remove blank lines I introduced in controller/test_case.rb.

Unit tests for FlashHash#to_session_value.

Put in a compatibility layer to accept FlashHash serializations from Rails 3.0+.

Test that Rails 3.2 session flashes are correctly converted to the new format.

Remove code path for processing Rails 3.0 FlashHashes since they can no longer deserialize.
…k the absence of attributes.

Add `ActiveModel::Errors#add_on_present` method. Adds error messages to present attributes.
@robotex82
Copy link
Contributor Author

This looks ....wrong?!

@carlosantoniodasilva
Copy link
Member

Looks like there are some extra commits now :)

@robotex82
Copy link
Contributor Author

OT: Time to read a book about GIT. Suggestions?

@steveklabnik
Copy link
Member

http://progit.org

@robotex82
Copy link
Contributor Author

I read the section on rebasing, but I don't get this right. :(

How do we get this right? Should I create a completely new pull request?

@steveklabnik
Copy link
Member

@steveklabnik
Copy link
Member

@robotex82 Because it's been a month, and you're obviously having some trouble, and I had some time this morning, I took care of this.

You can see the commit here: d72a07f

I also took care of @frodsan 's comment about the 1.9 hash syntax, and double checked @carlosantoniodasilva 's comments about blank lines.

Thank you for your pull request, hopefully you will overcome the git beast next time ;)

@wilmoore
Copy link

@robotex82: I highly recommend the Mastering Git series by @tlberglund and @matthewmccullough:

Full Disclosure: I know them both personally (and I appear in one of the videos) so I am a bit biased -- but trust me, watch these videos a few times through and you will level-up in your git-fu for sure.

@robotex82
Copy link
Contributor Author

Awesome, thank you guys! ❤️

Sorry for the late answer, but I've been very busy!

While checking the commit, I discovered, that the CHANGELOG is messed up a little bit:

Actual:

*   Add `ActiveModel::Validations::AbsenceValidator`, a validator to check the
    absence of attributes.

        class Person
          include ActiveModel::Validations

          attr_accessor :first_name
          validates_absence_of :first_name
        end

        person = Person.new
        person.first_name = "John"
        person.valid?
        # => false
        person.errors.messages
        # => {:first_name=>["must be blank"]}

    *Roberto Vasquez Angel*

*   `[attribute]_changed?` now returns `false` after a call to `reset_[attribute]!`

Shouldn't it be:

    *Roberto Vasquez Angel*

*   Add `ActiveModel::Validations::AbsenceValidator`, a validator to check the
    absence of attributes.

        class Person
          include ActiveModel::Validations

          attr_accessor :first_name
          validates_absence_of :first_name
        end

        person = Person.new
        person.first_name = "John"
        person.valid?
        # => false
        person.errors.messages
        # => {:first_name=>["must be blank"]}

*   `[attribute]_changed?` now returns `false` after a call to `reset_[attribute]!`

How do we get this right?

Thanks again!

@carlosantoniodasilva
Copy link
Member

Not sure what you mean, but the name goes after the change, at the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet