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

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

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

pvdb
Copy link
Contributor

@pvdb pvdb commented Dec 8, 2024

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
28 checks passed
@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
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants