Add a `required` option to the model generator #16062

Merged
merged 1 commit into from Aug 18, 2014

Conversation

Projects
None yet
7 participants
@sgrif
Member

sgrif commented Jul 5, 2014

Syntax was chosen to follow the passing of multiple options to
decimal/numeric types. Curly braces, and allowing any of ,, ., or
- to be used as a separator to avoid the need for shell quoting. (I'm
intending to expand this to all columns, but that's another PR.

The required option will cause 3 things to change. required: true
will be added to the association. null: false will be added to the
column in the migration. add_foreign_key :table, :foreign_table will
be added if the association is not polymorphic.

Examples:

rails g model Account supplier:references{required}
rails g model Account "supplier:references{required,polymorphic}"
rails g model Account supplier:references{polymorphic-required}
rails g model Account supplier:references{polymorphic.required}

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 5, 2014

Member

/cc @dhh

Member

sgrif commented Jul 5, 2014

/cc @dhh

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 5, 2014

Member

We need to update the migration guides with this option.

Member

rafaelfranca commented Jul 5, 2014

We need to update the migration guides with this option.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jul 5, 2014

Contributor

add_foreign_key :table, :foreign_table will be added if the association is not polymorphic.

This should be done for any references statement that's not polymorphic, not just required ones. Foreign keys may be null, only invalid non-nulls are restricted.

Contributor

egilburg commented Jul 5, 2014

add_foreign_key :table, :foreign_table will be added if the association is not polymorphic.

This should be done for any references statement that's not polymorphic, not just required ones. Foreign keys may be null, only invalid non-nulls are restricted.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 5, 2014

Member

@egilburg That's a conversation that's still being had, no decision has been made one way or the other yet.

Member

sgrif commented Jul 5, 2014

@egilburg That's a conversation that's still being had, no decision has been made one way or the other yet.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jul 5, 2014

Member

For the record, I'm with @egilburg: whether we generate FKs at all will presumably be a generator config option (regardless of its chosen default), but I believe that choice is unrelated to whether the value is required.

Member

matthewd commented Jul 5, 2014

For the record, I'm with @egilburg: whether we generate FKs at all will presumably be a generator config option (regardless of its chosen default), but I believe that choice is unrelated to whether the value is required.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 5, 2014

Member

I'm 👍 on FKs all the time as well. It sounds like that's the way most are leaning, I'll pull that out to a separate PR.

Member

sgrif commented Jul 5, 2014

I'm 👍 on FKs all the time as well. It sounds like that's the way most are leaning, I'll pull that out to a separate PR.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jul 7, 2014

Member

Looks good to me, but let's standardize on: rails g model Account supplier:references{required,polymorphic} -- don't like having 3 different splitters.

Member

dhh commented Jul 7, 2014

Looks good to me, but let's standardize on: rails g model Account supplier:references{required,polymorphic} -- don't like having 3 different splitters.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 7, 2014

Member

@dhh That used to be the only splitter. The issue is using , as a splitter requires shell escaping.

Member

sgrif commented Jul 7, 2014

@dhh That used to be the only splitter. The issue is using , as a splitter requires shell escaping.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jul 7, 2014

Member

Ah. Bummer. Okay, then let's use for only if that works.

On Jul 7, 2014, at 16:54, Sean Griffin notifications@github.com wrote:

@dhh That used to be the only splitter. The issue is using , as a splitter requires shell escaping.


Reply to this email directly or view it on GitHub.

Member

dhh commented Jul 7, 2014

Ah. Bummer. Okay, then let's use for only if that works.

On Jul 7, 2014, at 16:54, Sean Griffin notifications@github.com wrote:

@dhh That used to be the only splitter. The issue is using , as a splitter requires shell escaping.


Reply to this email directly or view it on GitHub.

@robin850 robin850 added this to the 4.2.0 milestone Jul 8, 2014

Add a `required` option to the model generator
Syntax was chosen to follow the passing of multiple options to
decimal/numeric types. Curly braces, and allowing any of `,`, `.`, or
`-` to be used as a separator to avoid the need for shell quoting. (I'm
intending to expand this to all columns, but that's another PR.

The `required` option will cause 2 things to change. `required: true`
will be added to the association. `null: false` will be added to the
column in the migration.
@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Aug 8, 2014

Member

This has been updated.

Member

sgrif commented Aug 8, 2014

This has been updated.

rafaelfranca added a commit that referenced this pull request Aug 18, 2014

Merge pull request #16062 from sgrif/sg-required-generators
Add a `required` option to the model generator

@rafaelfranca rafaelfranca merged commit 76883f9 into rails:master Aug 18, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
@djbender

This comment has been minimized.

Show comment
Hide comment
@djbender

djbender Aug 25, 2014

@sgrif @rafaelfranca Did the migration docs get updated for this yet?

@sgrif @rafaelfranca Did the migration docs get updated for this yet?

sachin004 added a commit to sachin004/rails that referenced this pull request Dec 13, 2014

sivagollapalli added a commit to sivagollapalli/rails that referenced this pull request Dec 29, 2014

@sgrif sgrif deleted the sgrif:sg-required-generators branch Mar 14, 2018

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