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 an option to include columns with a not null contraint #28

Closed
wants to merge 1 commit into from

Conversation

dwa012
Copy link

@dwa012 dwa012 commented Dec 21, 2015

No description provided.

@rubiety
Copy link
Owner

rubiety commented Dec 21, 2015

From an API point of view, I'm not sure I like using the key any here - it's too vague and doesn't literally mean what it implies ("any column", since we're still restricting by type and with content_columns, per discussion on #27). Maybe :nullables_only => false or something more descriptive? Not sure I have a good key in mind myself.

@dwa012
Copy link
Author

dwa012 commented Dec 21, 2015

@rubiety I am not the best at naming keys. I had made the change for a need that I had.
I think :nullables_only => false sounds good to me.

@jibiel
Copy link

jibiel commented Dec 21, 2015

Yep, this is a tricky one. Can't think of anything more descriptive myself, so 👍 for nullables_only: false / skip_nullables: false.

@dgobaud
Copy link

dgobaud commented Dec 22, 2015

Looks good just need to make sure it works globally such as

ActiveRecord::Base.nilify_blanks : skip_nullables => flase

@jibiel
Copy link

jibiel commented Dec 22, 2015

# config/initializers/nilify_blanks.rb
ActiveRecord::Base.nilify_blanks skip_nullables: false

Works for me on rails 4.2.5!
I think it always worked and some users (incl. me) just didn't expect it will be skipping null: false fields silently.

@jibiel
Copy link

jibiel commented Jan 9, 2016

@rubiety Any updates on that?

@rubiety
Copy link
Owner

rubiety commented Jan 10, 2016

Would be happy to accept a pull request implementing the above with appropriate tests, from the original author or otherwise.

rubiety added a commit that referenced this pull request Dec 23, 2017
@rubiety
Copy link
Owner

rubiety commented Dec 23, 2017

I have implemented this myself in commit c5a4af9 to master with tests, closing this pull request.

@rubiety rubiety closed this Dec 23, 2017
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

4 participants