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

Schema expands to property instead of property! #71

Closed
vladfaust opened this issue Feb 25, 2019 · 0 comments
Closed

Schema expands to property instead of property! #71

vladfaust opened this issue Feb 25, 2019 · 0 comments
Assignees
Labels
breaking Breaking change rfc Request For Comments
Projects
Milestone

Comments

@vladfaust
Copy link
Member

Currently this code:

class User
  schema users do
    pkey id : Int32
    type name : String
  end
end

Expands to "bang" properties:

class User
  property! id : Int32
  property! name : String
end

Which allows to call the properties assuming that they are not nil:

user = User.new(id: 42)
pp user.id == pp user.id?.not_nil! # OK

user = User.new(id: nil)
pp user.id # NillAssertion error in runtime

It enables writing less code, but makes becomes vulnerable to human errors like "I forgot to query id when fetching a user".

So, should the schema expand to property instead?

user = User.new(id: 42)
pp user.id # Compile-time is Int32 | Nil
pp user.id.not_nil! # Now it is Int32 only

It would make the code more safe, as the developer would be implied to explicitly state that a property is not nil, which would take some attention. He would think:

Ugh, its nilable in compile time. Oh, yes, that means that I can forget fetching it from DB. So I'd take time ensuring that the query preloads this property and only then add .not_nil! to the call.

However, it brings more bloated code with .not_nil!s everywhere. Also working with references becomes literally a nightmare:

user.id.not_nil!
post.author.not_nil!.id.not_nil!

Moreover, it's still not 100% bulletproof to human errors, as the developer may automatically type .not_nil! and still forgetting to actually preload a property.

WDYT?

@vladfaust vladfaust added rfc Request For Comments breaking Breaking change labels Feb 25, 2019
@vladfaust vladfaust added this to To do in Kanban via automation Feb 27, 2019
@vladfaust vladfaust self-assigned this Feb 27, 2019
@vladfaust vladfaust moved this from To do to In progress in Kanban Feb 27, 2019
@vladfaust vladfaust added this to the next minor milestone Feb 27, 2019
vladfaust added a commit that referenced this issue Feb 27, 2019
vladfaust added a commit that referenced this issue Feb 27, 2019
vladfaust added a commit that referenced this issue Mar 10, 2019
All Query methods are type-safe now. It is actually a hack
and relies on models having explicit `def foo : Type | Nil`
getters for **all** instance variables. Otherwise, query
builder methods would not work.

Related to #71, because in cd8c676 models had `property!`
generated for `not_null: true` fields. Now all instance
variables have `property : Type | Nil` generated instead
Kanban automation moved this from In progress to Done Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change rfc Request For Comments
Projects
Kanban
  
Done
Development

No branches or pull requests

1 participant