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

MONGOID-4741 Include timestamps in generated model by default #4631

Merged
merged 3 commits into from May 28, 2019
Merged

MONGOID-4741 Include timestamps in generated model by default #4631

merged 3 commits into from May 28, 2019

Conversation

Shigeyuki-fukuda
Copy link
Contributor

@Shigeyuki-fukuda Shigeyuki-fukuda commented May 15, 2019

Summary

The principal changes are as follows.

  • Remove -timestamp options from the files generated when use rails g model command
  • Add include Mongoid::Timestamps instead of it.

@johnnyshields
Copy link
Contributor

ActiveRecord still supports this option. This PR should be rejected in order to be consistent with ActiveRecord
https://github.com/rails/rails/blob/master/activerecord/lib/rails/generators/active_record/model/model_generator.rb

@Shigeyuki-fukuda
Copy link
Contributor Author

@johnnyshields
Thank you for your comment.
Is it OK if I restore it?

lib/rails/generators/mongoid/model/model_generator.rb

- class_option :timestamps, type: :boolean

@p-mongo
Copy link
Contributor

p-mongo commented May 16, 2019

According to https://github.com/rails/rails/blob/master/activerecord/lib/rails/generators/active_record/migration/templates/create_table_migration.rb.tt the timestamps are not defined by default, but must be requested via the timestamps option. I can see the convenience of having timestamps be added by default, but at the same time the argument can be made for maintaining ActiveRecord compatibility which does not add them by default. @Shigeyuki-fukuda can you reference any sentiment in the Rails community that the timestamps should be added to models by default?

@johnnyshields
Copy link
Contributor

@p-mongo let's close this. It can be re-opened if ActiveRecord makes a change.

@Shigeyuki-fukuda
Copy link
Contributor Author

Shigeyuki-fukuda commented May 17, 2019

@p-mongo

can you reference any sentiment in the Rails community that the timestamps should be added to models by default?

No I can't. 😓
I understood what you said.
I will think about it myself again.

@FumiyaShibusawa
Copy link

FumiyaShibusawa commented May 17, 2019

According to README of rails generate model command, timestamps are added by default, well tbh I've never been conscious of timestamps when I generate ActiveRecord model.

Attribute pairs are field:type arguments specifying the
model's attributes. Timestamps are added by default, so you don't have to
specify them by hand as 'created_at:datetime updated_at:datetime'.

Here is the output of README.

$ rails g model | cat
Running via Spring preloader in process 281
Usage:
  rails generate model NAME [field[:type][:index] field[:type][:index]] [options]

Options:
      [--skip-namespace], [--no-skip-namespace]  # Skip namespace (affects only isolated applications)
      [--force-plural], [--no-force-plural]      # Forces the use of the given model name
  -o, --orm=NAME                                 # ORM to be invoked
                                                 # Default: active_record

ActiveRecord options:
      [--migration], [--no-migration]        # Indicates when to generate migration
                                             # Default: true
      [--timestamps], [--no-timestamps]      # Indicates when to generate timestamps
                                             # Default: true
      [--parent=PARENT]                      # The parent class for the generated model
      [--indexes], [--no-indexes]            # Add indexes for references and belongs_to columns
                                             # Default: true
      [--primary-key-type=PRIMARY_KEY_TYPE]  # The type for primary key
  -t, [--test-framework=NAME]                # Test framework to be invoked
                                             # Default: rspec

Rspec options:
  [--fixture], [--no-fixture]   # Indicates when to generate fixture
  [--fixture-replacement=NAME]  # Fixture replacement to be invoked
                                # Default: factory_bot

Runtime options:
  -f, [--force]                    # Overwrite files that already exist
  -p, [--pretend], [--no-pretend]  # Run but do not make any changes
  -q, [--quiet], [--no-quiet]      # Suppress status output
  -s, [--skip], [--no-skip]        # Skip files that already exist

Description:
    Stubs out a new model. Pass the model name, either CamelCased or
    under_scored, and an optional list of attribute pairs as arguments.

    Attribute pairs are field:type arguments specifying the
    model's attributes. Timestamps are added by default, so you don't have to
    specify them by hand as 'created_at:datetime updated_at:datetime'.

    As a special case, specifying 'password:digest' will generate a
    password_digest field of string type, and configure your generated model and
    tests for use with Active Model has_secure_password (assuming the default ORM
    and test framework are being used).

    You don't have to think up every attribute up front, but it helps to
    sketch out a few so you can start working with the model immediately.

    This generator invokes your configured ORM and test framework, which
    defaults to Active Record and TestUnit.

    Finally, if --parent option is given, it's used as superclass of the
    created model. This allows you create Single Table Inheritance models.

    If you pass a namespaced model name (e.g. admin/account or Admin::Account)
    then the generator will create a module with a table_name_prefix method
    to prefix the model's table name with the module name (e.g. admin_accounts)

Available field types:
.
.
.

can you reference any sentiment in the Rails community that the timestamps should be added to models by default?

@p-mongo I'm not quite sure but I'll try looking into Rails' codebase that passes true to :timestamps option. 👍

@FumiyaShibusawa
Copy link

FWIW, as for the consistency with ActiveRecord, setting the default value true to :timestamps option would be more suited, not just deleting if condition, which would still give --no-timestamps option to users.

@johnnyshields
Copy link
Contributor

@FumiyaShibusawa let's make this PR match what Rails does.

@p-mongo
Copy link
Contributor

p-mongo commented May 20, 2019

It is not immediately clear to me how Rails accomplishes the default of true for timestamps, since that doesn't seem to be set anywhere, but in any event the same behavior in Mongoid can be accomplished, as far as I can tell, by adding default: true to https://github.com/mongodb/mongoid/blob/master/lib/rails/generators/mongoid/model/model_generator.rb#L14.

@p-mongo
Copy link
Contributor

p-mongo commented May 21, 2019

@Shigeyuki-fukuda If Mongoid's generator is changed to have default: true as I mentioned in the previous comment, would this fulfill your proposal of having timestamps be added to models by default?

@johnnyshields
Copy link
Contributor

@p-mongo we should copy the Rails options:

 [--timestamps], [--no-timestamps]

@Shigeyuki-fukuda
Copy link
Contributor Author

@p-mongo @johnnyshields
How about this ? 31c29e2

@p-mongo
Copy link
Contributor

p-mongo commented May 23, 2019

Does class_option :timestamps, type: :boolean, default: true not work?

@Shigeyuki-fukuda
Copy link
Contributor Author

Shigeyuki-fukuda commented May 24, 2019

@p-mongo

Does class_option :timestamps, type: :boolean, default: true not work?

Yes, it does.
The default: true worked.
The result is exactly what you said.
I fixed it. af9cde5

@johnnyshields
Copy link
Contributor

This works for me let's merge.

@p-mongo p-mongo changed the title Include timestamps in generated model MONGOID-4741 Include timestamps in generated model May 28, 2019
@p-mongo p-mongo changed the title MONGOID-4741 Include timestamps in generated model MONGOID-4741 Include timestamps in generated model by default May 28, 2019
@p-mongo p-mongo requested a review from saghm May 28, 2019 17:43
@p-mongo p-mongo merged commit 0093682 into mongodb:master May 28, 2019
@p-mongo
Copy link
Contributor

p-mongo commented May 28, 2019

Thank you for the patch, this will be included in Mongoid 7.1.

@Shigeyuki-fukuda Shigeyuki-fukuda deleted the default_timestamp_include branch May 29, 2019 00:58
@Shigeyuki-fukuda
Copy link
Contributor Author

Shigeyuki-fukuda commented May 29, 2019

@johnnyshields @p-mongo @saghm
Thank you for checking this PR. 😄

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