search

asok / projectile-rails

Emacs Rails mode based on projectile
258 stars 59 forks source link

Faster (non-recursive) projectile-rails-goto-file-at-point #103

Closed ejoubaud closed 8 years ago

ejoubaud commented 8 years ago

This should address #102. More specifically it implements the suggestion in https://github.com/asok/projectile-rails/issues/102#issuecomment-247577166

Instead of using f-entries to check every file in every subdir of the code directories (app/ and lib/ basically) see if they match the constant file name, this only tests the constant file name against:

  1. the dir matching the current file name (e.g. app/models/purchase if I'm in app/models/purchase.rb)
  2. each parent dir of the current file
  3. each code dir (but not their subdirs)

E.g.:

Any of those files that exists will be returned as a choice.

I reckon this should work in all the cases that don't break Rails autoload, and it's much much faster (on my fast machine, it takes the operation from several seconds to almost instantaneous)

Let me know if you'd like some benchmarks.

ejoubaud commented 8 years ago

I also committed the other solution I mentioned in #102 in c0254ab (still recursive and slower than this one, only slightly faster than the original), before replacing it with the new faster one in 538abbe. In case you prefer that one, it's just one revert away.

ejoubaud commented 8 years ago

The failing tests aren't my doing, they're failing on master too.

asok commented 8 years ago

Hey @ejoubaud sorry for the late response. I was on holidays.

The implementation I made was in fact the simplest and most obvious. I did not care about the performance. So it's great you've taken the effort to address it.

The failing tests aren't my doing, they're failing on master too.

They pass on my localhost. In fact if you look on Travis the errors are cryptic - they fail though it seems that they should pass.

I reckon this should work in all the cases that don't break Rails autoload

There's this file for testing that. As long as it passes it should be alright.

Your implementation is good but if you ask me I would make it more readable by introducing let* and by making the anonymous function not mutating the local variable (at least when f-traverse-upwards is not used). Roughly I mean something like this (I did not test it though):

  (let* ((code-dirs (-filter #'f-exists? (-map #'projectile-rails-expand-root (projectile-rails--code-directories))))
         (file-name (format "%s.rb" (projectile-rails-declassify name)))
         (filter-file (lambda (dir-name)
                        (let ((full-filename (f-join dir-name file-name)))
                          (when (f-exists? full-filename)
                            (f-canonical full-filename)))))
         (choices
          (-uniq
           (-non-nil
            (-flatten
             (list
              ;; Look for file in current file namespace
              (filter-file (f-no-ext buffer-file-name))
              ;; Look for file in code directories
              (-map code-dirs filter-file)
              ;; Look for file in local namespace hierarchy
              (let ((files '()))
                (f-traverse-upwards (lambda (parent-dir)
                                      (push (filter-file parent-dir) files)
                                      (equal (f-canonical (f-slash (projectile-rails-root))) (f-canonical (f-slash parent-dir))))
                                    (f-dirname buffer-file-name))
                files))))))))

Do you think you could refactor it in such fashion?

asok commented 8 years ago

Many thanks!

ejoubaud commented 8 years ago

@asok: Woops, I meant to push a new commit and a comment after but got sidetracked. You may want to wait before publishing the release as, testing this, I uncovered a regression with class names starting with a ::. They probably worked with the old version but not with this one. I'll put up another PR addressing that, hopefully later today.

asok commented 8 years ago

Ok cool

ejoubaud commented 8 years ago

@asok: Actually it seems the feature never worked with ::Classname never woked. projectile-rails-goto-file-at-point wouldn't match those as a constant (or anything for that matter).

I tried to fix this this with ((string-match-p "^\\(::\\)?[A-Z]" name) instead of ((string-match-p "^[A-Z]" name), but then projectile-rails-name-at-point passes the symbol at point through projectile-rails-sanitize-name, which strips the first colon in the leading ::, resulting in :Classname. That will then mess with projectile-rails-declassify, which will turn that into :classname (instead of /classname with ::Classname), so we won't be able to match the file.

Do you happen to know why projectile-rails-sanitize-name turns leading double colon into a single colon? If I could remove that part I could put up a PR to support absolute namespaces with leading ::, but I guess it must do that for a reason.

Anyway, TL;DR not an actual regression, feel free to move forward with the release :+1:

asok commented 8 years ago

Do you happen to know why projectile-rails-sanitize-name turns leading double colon into a single colon?

@ejoubaud I think the culprit might be here basically this when clause checks if it's a ruby's symbol. So given line belongs_to :user the function sanitizes :user to user I think if we would add an extra when clause with a check like (s-starts-with? "::" name) we could catch the absolute constant declaration. There are also tests for that function here.

ejoubaud commented 8 years ago

That's it thanks. Pushed the PR in #104 :)