Skip to content

refactor: use $LOADED_FEATURES built-in instead of $"#605

Merged
hsbt merged 1 commit intoruby:masterfrom
pvdb:loaded_features_builtin
Dec 9, 2024
Merged

refactor: use $LOADED_FEATURES built-in instead of $"#605
hsbt merged 1 commit intoruby:masterfrom
pvdb:loaded_features_builtin

Conversation

@pvdb
Copy link
Contributor

@pvdb pvdb commented Dec 8, 2024 •
edited
Loading

tl;dr - replace$" with the equivalent - but much more descriptive - $LOADED_FEATURES Ruby built-in ... and work around an apparent parsing bug in "universal" ctags while we're at it (see details below)

Thanks @hsbt 🙏

I noticed that quite a few methods defined in lib/rake/application.rb were missing from a tags file generated by the latest version of "universal" ctags.

On closer examination, it turns out that all methods defined after the rake_require method (defined here in the Ruby source file) don't get indexed by ctags:

    def rake_require(file_name, paths=$LOAD_PATH, loaded=$") # :nodoc:
      ...
    end

After some experimentation - and most likely due to a parsing bug in ctags - it turns out that Ruby's $" built-in is hmmm "confusing" ctags and causing it to generate a partial list of tags; using $LOADED_FEATURES instead of $" fixes the issue:

    def rake_require(file_name, paths=$LOAD_PATH, loaded=$LOADED_FEATURES) # :nodoc:
      ...
    end

$LOADED_FEATURES used to be an alias for $" defined in English.rb, but in ruby 1.9.3 (ruby/ruby@6dcbfbc) it was removed as an alias and was implemented as a "proper" built-in, so no additional require is needed for this change (especially considering rake has been targeting ruby v2.3 and above).

And if nothing else - even putting the tags issue aside - this change makes the code more readable and self-explanatory IMO.

Finally, by diffing the output of running:

ctags -f - --sort=no lib/rake/application.rb

... on the master branch as well as this PR's branch, it's easy to see that using $LOADED_FEATURES indeed results in all Ruby methods getting indexed, even the ones coming after the"problematic" rake_require method:

$ git rev-diff master loaded_features_builtin ctags -f - --sort=no lib/rake/application.rb
--- git checkout master && { ctags -f - --sort=no lib/rake/application.rb ; } ;
+++ git checkout loaded_features_builtin && { ctags -f - --sort=no lib/rake/application.rb ; } ;
@@ -51,4 +51,17 @@
 select_tasks_to_show        lib/rake/application.rb  /^    def select_tasks_to_show(options, show_tasks, value) # :nodoc:$/;"                    f  class:Rake.Application
 select_trace_output         lib/rake/application.rb  /^    def select_trace_output(options, trace_option, value) # :nodoc:$/;"                   f  class:Rake.Application
 handle_options              lib/rake/application.rb  /^    def handle_options(argv) # :nodoc:$/;"                                                f  class:Rake.Application
-rake_require                lib/rake/application.rb  /^    def rake_require(file_name, paths=$LOAD_PATH, loaded=$") # :nodoc:$/;"                f  class:Rake.Application
+rake_require                lib/rake/application.rb  /^    def rake_require(file_name, paths=$LOAD_PATH, loaded=$LOADED_FEATURES) # :nodoc:$/;"  f  class:Rake.Application
+find_rakefile_location      lib/rake/application.rb  /^    def find_rakefile_location # :nodoc:$/;"                                              f  class:Rake.Application
+print_rakefile_directory    lib/rake/application.rb  /^    def print_rakefile_directory(location) # :nodoc:$/;"                                  f  class:Rake.Application
+raw_load_rakefile           lib/rake/application.rb  /^    def raw_load_rakefile # :nodoc:$/;"                                                   f  class:Rake.Application
+glob                        lib/rake/application.rb  /^    def glob(path, &block) # :nodoc:$/;"                                                  f  class:Rake.Application
+system_dir                  lib/rake/application.rb  /^    def system_dir # :nodoc:$/;"                                                          f  class:Rake.Application
+standard_system_dir         lib/rake/application.rb  /^      def standard_system_dir #:nodoc:$/;"                                                S  class:Rake.Application
+standard_system_dir         lib/rake/application.rb  /^      def standard_system_dir #:nodoc:$/;"                                                S  class:Rake.Application
+collect_command_line_tasks  lib/rake/application.rb  /^    def collect_command_line_tasks(args) # :nodoc:$/;"                                    f  class:Rake.Application
+default_task_name           lib/rake/application.rb  /^    def default_task_name # :nodoc:$/;"                                                   f  class:Rake.Application
+add_import                  lib/rake/application.rb  /^    def add_import(fn) # :nodoc:$/;"                                                      f  class:Rake.Application
+load_imports                lib/rake/application.rb  /^    def load_imports # :nodoc:$/;"                                                        f  class:Rake.Application
+rakefile_location           lib/rake/application.rb  /^    def rakefile_location(backtrace=caller) # :nodoc:$/;"                                 f  class:Rake.Application
+set_default_options         lib/rake/application.rb  /^    def set_default_options # :nodoc:$/;"                                                 f  class:Rake.Application
$ _

@pvdb pvdb changed the title Use $LOADED_FEATURES built-in instead of $" refactor: use $LOADED_FEATURES built-in instead of $" Dec 8, 2024
@hsbt
Copy link
Member

hsbt commented Dec 9, 2024

Maked sense. Thanks 👍

@hsbt hsbt merged commit cbea814 into ruby:master Dec 9, 2024
@pvdb pvdb deleted the loaded_features_builtin branch December 9, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pvdb @hsbt