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

Correctly mark all Model columns as nullable #206

Closed
wants to merge 0 commits into from

Conversation

wagenet
Copy link

@wagenet wagenet commented Dec 15, 2021

This is necessary since new instances will have nil values, even for required columns.

Fixes #205

@pocke
Copy link
Owner

pocke commented Dec 16, 2021

Thanks for your PR.
Unfortunately, the non-nullable column is by design. I think the non-nullable column is more useful than the nullable column. .find, .create and so on are used more often than .new, so if the columns are always nullable, we need to write unnecessary null-guard (e.g. user.name or raise) everywhere.

By the way, I'm planning another approach to solve this problem. I think we can solve the problem by changing new method's returned value. For example:

# user.rbs

class User < ApplicationRecord
  interface _NewUser
    def id: () -> Integer?
    def name: () -> String?
  end

  def self.new: (...) -> User & User::_NewUser
  def id: () -> Integer
  def name: () -> String
end

# Ruby code

User.find(1).name # => String
User.new().name   # => String?

What do you think of this approach?


I have worked nothing on it yet and probably I cannot make time for it this year. Trying to implement is welcome 🙌

(Probably I wrote the problem and the idea into some issue, but I couldn't find the issue 🙈 )

@wagenet
Copy link
Author

wagenet commented Dec 16, 2021

I think something like this probably makes sense. In TypeScript terms, a new record would be something like Partial<User>. The only catch I see here is that the record won't chance to a non-partial upon save, but I don't think there's really a solution for this in TS either.

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.

Incorrectly marks ActiveRecord accessors as being non-nil
2 participants