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

Prefer rails command over bin/rails #33229

Merged
merged 6 commits into from Jul 24, 2018

Conversation

Projects
None yet
8 participants
@albertoalmagro
Copy link
Contributor

albertoalmagro commented Jun 26, 2018

Summary

Currently we are alternatively recommending the use of rails or bin/rails within guides and USAGE guidelines. In some files we recommend using rails, in others bin/rails, and in some cases we even have both options mixed together in the same file.

As exposed in @matthewd 's comment in #33203:

rails looks for, and runs, bin/rails if it's present

This PR standardizes on the shorter / more easily typed spelling railsas we discussed in the above mentioned PR.

Thanks!!

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Jun 26, 2018

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jun 26, 2018

@sikachu
Copy link
Member

sikachu left a comment

Looks like there were some legit failures on CI.

We might need to keep using bin/rails for parts in Railtie test where it's not actually documentation.

@matthewd
Copy link
Member

matthewd left a comment

Thanks for doing this! Seeing the inconsistency through both PRs' diffs really drives the point home.

guides/source/command_line.md Outdated
@@ -21,7 +21,7 @@ There are a few commands that are absolutely critical to your everyday usage of

* `rails console`
* `rails server`
* `bin/rails`
* `rails`

This comment has been minimized.

@matthewd

matthewd Jun 27, 2018

Member

This sounds odd.. should probably just go away (previously rake?)

guides/source/command_line.md Outdated
---------

Since Rails 5.0+ has rake commands built into the rails executable, `bin/rails` is the new default for running commands.
Since Rails 5.0+ has rake commands built into the rails executable, `rails` is the new default for running commands.

This comment has been minimized.

@matthewd

matthewd Jun 27, 2018

Member

Okay, yeah.. this reads super strangely. In the middle of a list of rails subcommands, we stop and talk about a different set of rails subcommands.

If you're up for it, I think this deserves some more aggressive surgery, to better integrate previously-rake commands into the outer list (based on their individual predicted-frequency-of-use).

This comment has been minimized.

@albertoalmagro

albertoalmagro Jun 27, 2018

Author Contributor

Sure I am. I will let you both now when I am back with changes done. Thank you both for your reviews and great hints!

guides/source/upgrading_ruby_on_rails.md Outdated
@@ -257,16 +257,18 @@ it.

`debugger` is not supported by Ruby 2.2 which is required by Rails 5. Use `byebug` instead.

### Use bin/rails for running tasks and tests
### Use rails or bin/rails for running tasks and tests

This comment has been minimized.

@matthewd

matthewd Jun 27, 2018

Member

I think backticks are allowed in headings, so it seems fine to make this [just] `rails`. No need to call out bin/rails here; while this is about using it more often, people have already long been using it for other subcommands. (Likewise the sentence below.)

If we don't already address the version-handling etc elsewhere, I'd say the command line guide is the place to spend a paragraph on that (including the equivalence to bin/rails, and the fact it relies on that file).

activesupport/lib/active_support/key_generator.rb Outdated
@@ -59,7 +59,7 @@ def ensure_secret_secure(secret)
if secret.blank?
raise ArgumentError, "A secret is required to generate an integrity hash " \
"for cookie session data. Set a secret_key_base of at least " \
"#{SECRET_MIN_LENGTH} characters in via `bin/rails credentials:edit`."
"#{SECRET_MIN_LENGTH} characters in via `rails credentials:edit`."

This comment has been minimized.

@matthewd

matthewd Jun 27, 2018

Member

(existing) typo? "in via" doesn't sound right.

railties/lib/rails/commands/credentials/credentials_command.rb Outdated
else
"No credentials have been added yet. Use bin/rails credentials:edit to change that."
"No credentials have been added yet. Use rails credentials:edit to change that."

This comment has been minimized.

@matthewd

matthewd Jun 27, 2018

Member

Messages like this should really be using backticks to set the command off from the surrounding sentence (or a "words words words: command until end of line" form). It was not-great before, but I think the obvious command-ness of bin/rails at least made the start clear... rails slides right into the sentence.

This and encrypted_command below seem to be the only ones -- might as well fix them.

railties/lib/rails/commands/server/server_command.rb Outdated
@@ -98,7 +98,7 @@ def log_to_stdout
end

def restart_command
"bin/rails server #{ARGV.join(' ')}"
"rails server #{ARGV.join(' ')}"

This comment has been minimized.

@matthewd

matthewd Jun 27, 2018

Member

While we might talk about running rails, I think we're better off still going straight to bin/rails when we're the ones doing the running. That saves us a few milliseconds of indirection to get back to the same place, and avoids trouble if anyone has an unusual PATH setup -- I'm happy to let such people handle mental translation of rails to whatever special command they need, but avoidably breaking because of it seems less good.

railties/lib/rails/generators/actions.rb Outdated
@@ -215,7 +215,7 @@ def generate(what, *args)
log :generate, what
argument = args.flat_map(&:to_s).join(" ")

in_root { run_ruby_script("bin/rails generate #{what} #{argument}", verbose: false) }
in_root { run_ruby_script("rails generate #{what} #{argument}", verbose: false) }

This comment has been minimized.

@matthewd

matthewd Jun 27, 2018

Member

My above thinking would apply here too... though I note the below rails_command seems happy relying on the unqualified name 🤠

railties/lib/rails/generators/rails/app/templates/bin/setup.tt Outdated
@@ -28,12 +28,12 @@ chdir APP_ROOT do
# end

puts "\n== Preparing database =="
system! 'bin/rails db:setup'
system! 'rails db:setup'

This comment has been minimized.

@matthewd

matthewd Jun 27, 2018

Member

I'm definitely torn on this one... I'm again inclined to prefer the safer bin/rails (what if they don't have the gem installed globally, and the bundle is vendored?), but on the other hand, it is user-visible. I think safety might trump aesthetics here, though.

@albertoalmagro albertoalmagro changed the title Albertoalmagro/prefer rails command over bin rails Prefer rails command over bin/rails Jun 27, 2018

@albertoalmagro albertoalmagro changed the title Prefer rails command over bin/rails [WIP] Prefer rails command over bin/rails Jun 27, 2018

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jun 27, 2018

I have addressed feedback and fixed CI. I haven't squashed commits yet so that you can review them separately, but I have indicated the commits I want to squashed them into.

I still need to rewrite rails section at guides/source/command_line.md. I will come back with that later.

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:albertoalmagro/prefer-rails-command-over-bin-rails branch 4 times, most recently Jun 27, 2018

@albertoalmagro albertoalmagro changed the title [WIP] Prefer rails command over bin/rails Prefer rails command over bin/rails Jun 27, 2018

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:albertoalmagro/prefer-rails-command-over-bin-rails branch Jun 27, 2018

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jun 27, 2018

CI started to fail again but it doesn't seem to be related to this PR, I have retried it twice, once after rebasing but it persist, may some dependency service be down? https://travis-ci.org/rails/rails/jobs/397555590

@matthewd @sikachu All changes done. Please review again when you can.

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:albertoalmagro/prefer-rails-command-over-bin-rails branch Jun 28, 2018

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jun 28, 2018

I have already reorganized commits. This should be ready to go 🙂

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:albertoalmagro/prefer-rails-command-over-bin-rails branch Jun 28, 2018

@sikachu
Copy link
Member

sikachu left a comment

I have a few comments about "rails tasks" wording as I think we need to call them something else. (Rails subcommands?) Maybe we can refer to Git on what they use to call those subcommands.

guides/source/active_record_migrations.md Outdated
@@ -12,7 +12,7 @@ After reading this guide, you will know:

* The generators you can use to create them.
* The methods Active Record provides to manipulate your database.
* The bin/rails tasks that manipulate migrations and your schema.
* The rails tasks that manipulate migrations and your schema.

This comment has been minimized.

@sikachu

sikachu Jun 28, 2018

Member

Looks like we want to say "The rails commands" or "Rails subcommands" here?

guides/source/active_record_migrations.md Outdated
@@ -727,9 +727,9 @@ you will have to use `structure.sql` as dump method. See
Running Migrations
------------------

Rails provides a set of bin/rails tasks to run certain sets of migrations.
Rails provides a set of rails tasks to run certain sets of migrations.

This comment has been minimized.

@sikachu

sikachu Jun 28, 2018

Member

Another "rails tasks" here that needs to be changed.

guides/source/active_record_migrations.md Outdated

The very first migration related bin/rails task you will use will probably be
The very first migration related rails task you will use will probably be

This comment has been minimized.

@sikachu

sikachu Jun 28, 2018

Member

Another "rails tasks" here that needs to be changed.

guides/source/autoloading_and_reloading_constants.md Outdated
* In **development**, you want quicker startup with incremental loading of application code. So `eager_load` should be set to `false`, and Rails will autoload files as needed (see [Autoloading Algorithms](#autoloading-algorithms) below) -- and then reload them when they change (see [Constant Reloading](#constant-reloading) below).
* In **production**, however you want consistency and thread-safety and can live with a longer boot time. So `eager_load` is set to `true`, and then during boot (before the app is ready to receive requests) Rails loads all files in the `eager_load_paths` and then turns off auto loading (NB: autoloading may be needed during eager loading). Not autoloading after boot is a `good thing`, as autoloading can cause the app to be have thread-safety problems.
* In **test**, for speed of execution (of individual tests) `eager_load` is `false`, so Rails follows development behaviour.
* In **production**, however you want consistency and thread-safety and can live with a longer boot time. So `eager_load` is set to `true`, and then during boot (before the app is ready to receive requests) Rails loads all files in the `eager_load_paths` and then turns off auto loading (NB: autoloading may be needed during eager loading). Not autoloading after boot is a `good thing`, as autoloading can cause the app to be have thread-safety problems.

This comment has been minimized.

@sikachu

sikachu Jun 28, 2018

Member

Not really related to the main change, but I think it'd read better if we do

* In **production**, however, you want ...
guides/source/command_line.md Outdated
* `rails generate`
* `rails dbconsole`
* `rails new app_name`

All commands can run with `-h` or `--help` to list more information.

There is also a set of commands that in previous versions of Rails used to be invoked with `rake`. Since Rails 5.0+ `rails` is the new default for running these commands. Again, based on their individual predicted frequency of use, some of them are:

This comment has been minimized.

@matthewd

matthewd Jun 28, 2018

Member

Sorry to be difficult, but I was thinking of something a bit more drastic: this changed 3 versions ago; I don't think this documentation should include any indication which commands were once rake tasks, or even mention that change at all. The below items should be mixed into the above list, and the subheadings under "Former rake commands" should be top-level headings in their own right.

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jun 28, 2018

@sikachu @matthewd Thanks for your feedback!

@sikachu Good point. To me they would really be subcommands of the rails command, but as we have historically referred to rails serverand the likes as commands, I would substitute all references to task for command. This also makes sense with the idea of unifying all commands under rails.

@matthewd Sure! It definitely makes sense, I didn't understand it well, I will change it. Do you find the ones I "selected" meaningful? Do you miss any special command? Would you change their ordering?

I would propose to have this ordering:

  • rails console
  • rails server
  • rails generate
  • rails test
  • rails db:migrate
  • rails db:rollback
  • rails db:create
  • rails db:drop
  • rails routes
  • rails dbconsole
  • rails new app_name

Then, although I at least don't use them so often, I would tend to maintain other commands we currently have in the guide like about, assets, tmp, and so on, but maybe in an additional section in order not to excessively bloat the outer list. What do you think?

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Jun 29, 2018

Sounds good! I agree we don't actually want to put all the tasks into that top list -- it doesn't even have all the 'original' subcommands.

My only immediate opinion on the order you propose is to pull test above generate. Beyond that, I'm not sure that db:rollback and db:drop deserve equal billing to their positive counterparts... e.g. the current list mentions generate but not destroy?

Looking at it, I'm feeling pretty doubtful about the whole narrative structure of this guide, tbh. It feels constrained by the fact it's working through a new application, so it's stuck primarily describing how to use the command in a way that moves us forward to the next command, instead of exploring each command in isolation. Meanwhile, it's describing a sequence rather similar to that already addressed by the getting started guide, which this guide lists as a prerequisite. 😕

Changing that seems like a distraction from this PR, though. If we can for now just change enough to make all the commands sound like peers, I think that's a good place to draw the line: this PR is cleaning out various ways that the rake -> rails swap left evidence of which command was originally where (mostly in the form of bin/rails, but in that guide, with words and structure)... fundamentally revising a whole guide can wait for another day. 😅

@albertoalmagro albertoalmagro changed the title Prefer rails command over bin/rails [WIP] Prefer rails command over bin/rails Jun 29, 2018

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:albertoalmagro/prefer-rails-command-over-bin-rails branch 3 times, most recently Jun 29, 2018

@albertoalmagro albertoalmagro changed the title [WIP] Prefer rails command over bin/rails Prefer rails command over bin/rails Jun 29, 2018

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jun 29, 2018

Feedback applied!

@sikachu I have corrected the typo here e455554. Regarding references to rails tasks I have changed the ones you mentioned to rails commands. I have also searched throughout my changes and the whole code base for more. You can all I found and corrected here efb3470.

@matthewd I have applied your feedback here fcc1fbf. As you can see test is now above generate, and I leaved out the negative alternatives rollback and drop. I agree with you that changing the structure of that guide would be out of the scope of this PR. Nevertheless, I offer me volunteer to do it. Where could we discuss about it?

@sikachu @matthewd I also found out that the USAGE instructions of the existing commands still encouraged to use bin/rails. I have fixed that here eb8fcd88e6e69138ff694ce828c55c551db2d4c5.

I hope you like the changes. Thanks in advance for your feedback and time!

@matthewd
Copy link
Member

matthewd left a comment

This looks great! Love the task -> command change too -- this cleans up a huge number of papercuts where the "was once a rake task" distinction was still visible! Two tiny comments below.

On the structural change, I've opened #33270 so we can gather our thoughts for now, though I imagine most of the interesting discussion will come once we have a rough-cut PR to look at. ❤️

guides/source/command_line.md Outdated
@@ -255,10 +294,10 @@ $ bin/rails generate scaffold HighScore game:string score:integer

The generator checks that there exist the directories for models, controllers, helpers, layouts, functional and unit tests, stylesheets, creates the views, controller, model and database migration for HighScore (creating the `high_scores` table and fields), takes care of the route for the **resource**, and new tests for everything.

The migration requires that we **migrate**, that is, run some Ruby code (living in that `20130717151933_create_high_scores.rb`) to modify the schema of our database. Which database? The SQLite3 database that Rails will create for you when we run the `bin/rails db:migrate` command. We'll talk more about bin/rails in-depth in a little while.
The migration requires that we **migrate**, that is, run some Ruby code (living in that `20130717151933_create_high_scores.rb`) to modify the schema of our database. Which database? The SQLite3 database that Rails will create for you when we run the `rails db:migrate` command. We'll talk more about `rails db` in a little while.

This comment has been minimized.

@matthewd

matthewd Jul 1, 2018

Member

"We'll talk more about that command below." ?

guides/source/command_line.md Outdated
@@ -443,24 +438,24 @@ Database adapter sqlite3
Database schema version 20180205173523
```

### `assets`
### `rails assets`

This comment has been minimized.

@matthewd

matthewd Jul 1, 2018

Member

Let's make this rails assets:*, and same for db and tmp, just because they're namespaces only, with no top-level command.

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:albertoalmagro/prefer-rails-command-over-bin-rails branch Jul 1, 2018

@albertoalmagro
Copy link
Contributor Author

albertoalmagro left a comment

Thanks @matthewd ! Changes done, love to see you opened #33270 ❤️

albertoalmagro added some commits Jun 26, 2018

Recommend use of rails over bin/rails
As discussed in #33203 rails command already looks for, and runs,
bin/rails if it is present.

We were mixing recommendations within guides and USAGE guidelines,
in some files we recommended using rails, in others bin/rails and
in some cases we even had both options mixed together.
Fix typo 'in via'
Substitutes 'in via' for 'by running'
Homogenize rails commands and former rake tasks
This commit integrates most used previously rake commands into the
above outer list. Ordering is based on their individual predicted
frequency of use.

Separation between bin/rails tasks is also removed to display all
commands at the same level.

It also removes references to rake and tasks and substitutes them
for command.
Show rails instead of bin/rails on USAGE instructions
With this commit, rails commands usage instructions display now +rails+
instead of +bin/rails+ within their recommendations.
Substitute references to task for command
This commit substitutes references to rails/rake task for rails command

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:albertoalmagro/prefer-rails-command-over-bin-rails branch to 2e194d0 Jul 6, 2018

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jul 6, 2018

I've just fixed some conflicts that started after last merge.

@sikachu could you please review? Thanks!

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jul 24, 2018

@sikachu may be on holidays...

Would it possible for someone else to move this forward please? I would like to avoid fixing conflicts again 😉 Thanks!

@azbshiri

This comment has been minimized.

Copy link
Contributor

azbshiri commented Jul 24, 2018

@albertoalmagro for sure, I can.

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jul 24, 2018

@azbshiri thanks, but sorry, I didn't express myself well. I meant someone from the core team with merge rights.

@matthewd matthewd merged commit ec387c6 into rails:master Jul 24, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Jul 24, 2018

🤘

Thanks for this -- and for the poke 😅

@sikachu

This comment has been minimized.

Copy link
Member

sikachu commented Jul 24, 2018

Whoops, sorry about that. Thank you for taking care of this @matthewd.

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jul 25, 2018

Thank you both @sikachu @matthewd !!

@mattbrictson

This comment has been minimized.

Copy link
Contributor

mattbrictson commented Jul 29, 2018

Unfortunately rails is slower than bin/rails since the latter uses Spring to bypass some rubygems boot up time*.

$ time bin/rails r "puts 'hello'"
Running via Spring preloader in process 96435
hello

real	0m0.365s
user	0m0.185s
sys	0m0.061s
$ time rails r "puts 'hello'"
Running via Spring preloader in process 96525
hello

real	0m0.878s
user	0m0.589s
sys	0m0.162s

As you can see, rails is about a half-second slower, at least on my machine. It is likely a function of how many gems are installed (I have about 340).

Is this a concern?


* Specifically, when you type rails, rubygems has to locate and load the executable, using Gem.activate_bin_path, which takes some time:

$ time ruby -e 'Gem.activate_bin_path("railties", "rails", ">= 0.a")'

real	0m0.580s
user	0m0.438s
sys	0m0.125s
@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jul 29, 2018

Hi @mattbrictson, thanks for the benchmarks! To be sincere, speed was never a concern when we discussed this, but having seen the numbers, it is good to see that it is about half-second, because I am definitely slower than half-second when writing bin/ 😉

Leaving my slowness aside, our goal here was to be consistent with our recommendations. Sometimes we were recommending using bin/rails while others rails. As rails (among other stuff) is already looking for and executing bin/rails, we decided to be consistent by recommending always the shorter version. I hope that helps 🙏

@mattbrictson

This comment has been minimized.

Copy link
Contributor

mattbrictson commented Jul 30, 2018

I agree, that makes sense. I understand the need to make the documentation consistent.

Some Rails developers (myself included) have set up their shells so that ./bin is in their PATH (i.e. so that rails actually expands to ./bin/rails). Despite the fact that this is technically unnecessary and no longer called for in the documentation, I just wanted to point out that doing so will still result in a moderate speed boost.

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

albertoalmagro commented Jul 30, 2018

It is always great to have more information @mattbrictson, thanks again for sharing those benchmarks ❤️

@Fjan

This comment has been minimized.

Copy link
Contributor

Fjan commented Jul 30, 2018

On my (somewhat old) computer the difference between rails and bin/rails is 1.03 sec on a project with about 20 gems. For me time ruby -e 'Gem.activate_bin_path("railties", "rails", ">= 0.a")' is only 0.137s real so I don't think that's the explanation.

Where is rails spending all this time to locate the correct rails version?

albertoalmagro added a commit to albertoalmagro/rails that referenced this pull request Aug 1, 2018

[ci skip] Change references from Rake task to Rails command
This commit follows the path we started at commit #ea4f0e2
and continued at PR rails#33229.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.