Skip to content

Conversation

p8
Copy link
Member

@p8 p8 commented Jun 27, 2024

Legacy STATS_DIRECTORIES hook for Rake no longer works

STATS_DIRECTORIES is used by third parties to add directories to the statistics output. It's a global constant defined in a Rake file, that gets loaded anytime the Rake commands get loaded.

For example Rspec Rails adds spec directories in a prepended Rake task: https://github.com/rspec/rspec-rails/blob/8c17b4e5020a4d264e8a79e294c58b5c1ef2b005/lib/rspec/rails/tasks/rspec.rake#L43

Rake tasks only get loaded if no matching Thor task has been found. This means this constant is only available when the Rake commands have loaded.

New CodeStatistics.add_directory for Thor command

As the stats command has now been moved to a Thor task, calling bin/rails stats will no longer add directories to STATS_DIRECTORIES, as the Rake commands don't get loaded anymore.

To remove the dependency on Rake and avoid a global constant we can add an API to add directories: CodeStatistics.add_directory. STATS_DIRECTORIES is deprecated.

deprecate_constant couldn't be used here as that doesn't seem to work for the root namespace.

This was previously proposed/discussed in #49759, but I decided to resubmit it with the recent commit to make stats a Thor task.

Before

bin/rake stats adds custom directories (Model specs in this case):

bin/rake stats
DEPRECATION WARNING: `bin/rails stats` as rake task has been deprecated and will be removed in Rails 8.0.
Please use `bin/rails stats` as Rails command instead.

 (called from block in execute at .../gems/rake-13.1.0/lib/rake/task.rb:281)
DEPRECATION WARNING: `bin/rails stats` as rake task has been deprecated and will be removed in Rails 8.0.
Please use `bin/rails stats` as Rails command instead.

 (called from block in execute at .../gems/rake-13.1.0/lib/rake/task.rb:281)
+----------------------+--------+--------+---------+---------+-----+-------+
| Name                 |  Lines |    LOC | Classes | Methods | M/C | LOC/M |
+----------------------+--------+--------+---------+---------+-----+-------+
| Controllers          |      7 |      7 |       2 |       1 |   0 |     5 |
...
| System tests         |      0 |      0 |       0 |       0 |   0 |     0 |
| Model specs          |      5 |      4 |       0 |       0 |   0 |     0 |
+----------------------+--------+--------+---------+---------+-----+-------+
| Total                |    161 |    115 |      14 |       1 |   0 |   113 |
+----------------------+--------+--------+---------+---------+-----+-------+

bin/rails stats does not add custom directories (Model specs):

$ bin/rails stats
+----------------------+--------+--------+---------+---------+-----+-------+
| Name                 |  Lines |    LOC | Classes | Methods | M/C | LOC/M |
+----------------------+--------+--------+---------+---------+-----+-------+
| Controllers          |      7 |      7 |       2 |       1 |   0 |     5 |
...
| System tests         |      0 |      0 |       0 |       0 |   0 |     0 |
+----------------------+--------+--------+---------+---------+-----+-------+
| Total                |    156 |    111 |      14 |       1 |   0 |   109 |
+----------------------+--------+--------+---------+---------+-----+-------+

After

bin/rake stats adds directories (Model specs).

$ bin/rake stats
DEPRECATION WARNING: `STATS_DIRECTORIES` is deprecated and will be removed in Rails 8.1! Use `Rails.application.config.code_statistics.directories` instead. (called from block in execute at .../gems/rake-13.1.0/lib/rake/task.rb:281)
DEPRECATION WARNING: `bin/rake stats` as rake task has been deprecated and will be removed in Rails 8.1.
Please use `bin/rails stats` as Rails command instead.

 (called from block in execute at .../gems/rake-13.1.0/lib/rake/task.rb:281)
DEPRECATION WARNING: `bin/rake stats` as rake task has been deprecated and will be removed in Rails 8.1.
Please use `bin/rails stats` as Rails command instead.

 (called from block in execute at .../gems/rake-13.1.0/lib/rake/task.rb:281)
+----------------------+--------+--------+---------+---------+-----+-------+
| Name                 |  Lines |    LOC | Classes | Methods | M/C | LOC/M |
+----------------------+--------+--------+---------+---------+-----+-------+
| Controllers          |      7 |      7 |       2 |       1 |   0 |     5 |
...
| System tests         |      0 |      0 |       0 |       0 |   0 |     0 |
| Model specs          |      5 |      4 |       0 |       0 |   0 |     0 |
+----------------------+--------+--------+---------+---------+-----+-------+
| Total                |    161 |    115 |      14 |       1 |   0 |   113 |
+----------------------+--------+--------+---------+---------+-----+-------+

bin/rails stats adds directories added in a Raills.application.config.after_initialize block.

$ bin/rails stats
+----------------------+--------+--------+---------+---------+-----+-------+
| Name                 |  Lines |    LOC | Classes | Methods | M/C | LOC/M |
+----------------------+--------+--------+---------+---------+-----+-------+
| Controllers          |      7 |      7 |       2 |       1 |   0 |     5 |
...
| System tests         |      0 |      0 |       0 |       0 |   0 |     0 |
| Model specs          |      5 |      4 |       0 |       0 |   0 |     0 |
+----------------------+--------+--------+---------+---------+-----+-------+
| Total                |    161 |    115 |      14 |       1 |   0 |   113 |
+----------------------+--------+--------+---------+---------+-----+-------+
....

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@p8 p8 force-pushed the railties/deprecate-stats-directories branch 2 times, most recently from 38bf814 to c2b17c4 Compare June 27, 2024 11:28
@p8 p8 force-pushed the railties/deprecate-stats-directories branch from f9a8a5a to 6603a8d Compare June 27, 2024 12:20
@zzak zzak mentioned this pull request Jun 28, 2024
4 tasks
@p8
Copy link
Member Author

p8 commented Jun 28, 2024

Instead of having a config setting that requires a booted application, it would probably be better to have a clear API on CodeStatistics:

    CodeStatistics.add_directory("My Directory", "path/to/dir")

@p8 p8 force-pushed the railties/deprecate-stats-directories branch from 6603a8d to 44e7a0c Compare June 28, 2024 09:29
@p8
Copy link
Member Author

p8 commented Jun 28, 2024

I've changed the PR to use CodeStatistics.add_directory instead of config.code_statistics.directories.

@p8 p8 changed the title Deprecate STATS_DIRECTORIES in favor of `config.code_statistics.directories Deprecate STATS_DIRECTORIES in favor of CodeStatistics.add_directory Jun 28, 2024
@p8 p8 force-pushed the railties/deprecate-stats-directories branch 2 times, most recently from 25acec7 to ff2c5e3 Compare June 28, 2024 10:20
@p8 p8 changed the title Deprecate STATS_DIRECTORIES in favor of CodeStatistics.add_directory Add third party hook for rails stats Thor command Jul 1, 2024
@p8 p8 changed the title Add third party hook for rails stats Thor command Fix third party hook for rails stats Thor command Jul 11, 2024
@p8 p8 force-pushed the railties/deprecate-stats-directories branch 2 times, most recently from 57d61c6 to 0b279d4 Compare August 2, 2024 13:23
@p8 p8 force-pushed the railties/deprecate-stats-directories branch from 0b279d4 to 1ac3d37 Compare August 21, 2024 20:26
require "rails/code_statistics_calculator"
require "active_support/core_ext/enumerable"

class CodeStatistics # :nodoc:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is nodoc, we can't add a method we expect people to use on it and keep it nodoc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets remove the nodoc and document this class

Copy link
Member Author

@p8 p8 Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to document this class should it also be namespaced under Rails:: like most classes in railties/lib/rails?
Otherwise we'll have a top level constant CodeStatistics

Copy link
Member

@zzak zzak Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@p8 I think this should be inside the Rails:: constant, to match the path
Edit: Fixed in #52706 (Sorry I'm catching up)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙂

require "rails/code_statistics_calculator"
require "active_support/core_ext/enumerable"

class CodeStatistics # :nodoc:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets remove the nodoc and document this class

`STATS_DIRECTORIES` is used by third parties to add directories to the
statistics output. It's a global constant defined in a Rake file, that
gets loaded anytime the Rake commands get loaded.

For example Rspec Rails adds these in a prepended Rake task:
https://github.com/rspec/rspec-rails/blob/8c17b4e5020a4d264e8a79e294c58b5c1ef2b005/lib/rspec/rails/tasks/rspec.rake#L43

Rake tasks only get loaded if no matching Thor task has been found. This
means `STATS_DIRECTORIES` is only available when the Rake commands have
loaded.

As the stats command has now been moved to a Thor task, calling
`bin/rails stats` will no longer add directories to `STATS_DIRECTORIES`,
as the Rake commands don't get loaded anymore.

To remove the dependency on Rake and avoid a global constant we can add
an API to add directories: `CodeStatistics.add_directory`.
`STATS_DIRECTORIES` is deprecated.

`deprecate_constant` couldn't be used here as that doesn't seem to work
for the root namespace.

Co-authored-by: Earlopain <14981592+Earlopain@users.noreply.github.com>
@p8 p8 force-pushed the railties/deprecate-stats-directories branch from 1ac3d37 to 271467b Compare August 23, 2024 12:59
@rafaelfranca rafaelfranca merged commit 667a206 into rails:main Aug 23, 2024
2 of 3 checks passed
@p8 p8 deleted the railties/deprecate-stats-directories branch August 24, 2024 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants