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

add sort by path length & custom method #1442

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dradetsky
Copy link

@dradetsky dradetsky commented Aug 27, 2019

I wanted to be able to sort the list of files for projectile-find-file by the length of the full path. The reason is that if I want to find models/user.rb and I'm presented with

models/special_case/user_hamburger.rb
models/user.rb

I can only get the one I want by navigating in the completion window, which is cumbersome (or maybe there's another way, but I don't know it), rather than just doing more incremental completion. There's nothing to type to narrow down to just the file I want (or nothing short).

By contrast, if I'm presented with that list in reverse, and I actually am looking for user_hamburger.rb, I can get to it with incremental completion, and I can see what letters to type to get there.

I originally started out to add an option to sort the list by a custom method, so I could add my own sort-by-path-length method and use that. When I finished, I considered that enough other users would want to sort by path length that I might as well add an option to do that. So I did, and that's what I now use for my sort method, rather than the custom method.

I considered removing the sort-by-custom-method code, but thought that the maintainers might think both were useful. I can remove one or the other if the maintainers think it's too much.

NOTE: I don't really know how to write cask tests. I looked and I didn't see any tests for the existing sorting or even for projectile-project-files, so I presume not writing tests for my change is okay. I did manage to install cask & butterfly & confirm that I at least didn't break any existing tests.


Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
    i think so, kinda
  • You've added tests (if possible) to cover your change(s)
    arguably not possible
  • All tests are passing (make test)
  • The new code is not generating bytecode or M-x checkdoc warnings
    as far as i can tell, although checkdoc says all kinds of stuff is wrong so i could have missed something
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

Thanks!

@dradetsky
Copy link
Author

@bbatsov rebased/merged for conflicts

doc/configuration.md Outdated Show resolved Hide resolved
doc/configuration.md Outdated Show resolved Hide resolved

This should be the name of a predicate method usable by `cl-sort'"
:group 'projectile
:type 'symbol)
Copy link
Owner

Choose a reason for hiding this comment

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

You should also add a package-version property here saying 2.1

Copy link
Author

Choose a reason for hiding this comment

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

i did 2.1.0, since the others were all 3 digit

@bbatsov
Copy link
Owner

bbatsov commented Oct 6, 2019

The proposed changes look OK to me. I've added some small remarks.

@bbatsov
Copy link
Owner

bbatsov commented Oct 16, 2019

Looking at the failing build it seems there's a syntax error somewhere.

@stale
Copy link

stale bot commented Nov 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the Stale label Nov 26, 2020
@bbatsov
Copy link
Owner

bbatsov commented Dec 3, 2020

@dradetsky You'll also need to rebase on top of master.

@stale stale bot removed the Stale label Dec 3, 2020
@dradetsky
Copy link
Author

@bbatsov just came across this again (been kind of busy). I'll try to fix it soon. In the meantime, if you want to fix it yourself or just steal bits of it that's totally cool.

@bbatsov
Copy link
Owner

bbatsov commented May 24, 2021

@dradetsky Any chance of you rebasing/wrapping this up?

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

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.

2 participants