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

Support projectile-replace to select file extension on C-u #1778

Merged
merged 9 commits into from
Aug 28, 2022

Conversation

taquangtrung
Copy link
Contributor

@taquangtrung taquangtrung commented May 29, 2022

Hi,

I created this PR to enable users to select file name pattern and extension when performing projectile-replace with C-u.

This is to prevent projectile-replace from searching on very large files of a project (my Emacs hangs when projectile search on a 1MB Javascript library file, while I only wanted to search and replace on Rust code).

Currently, the file extension filtering is supported only by ag (using the option -G <file-ext-pattern>).

I don't use rg, hence I haven't been able to add the corresponding option for it (and also for ack and grep).

Can you take a look and advise if this PR can be merged?

Thank you!


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
  • [ ] You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

Thanks!

@bbatsov
Copy link
Owner

bbatsov commented May 30, 2022

I like the idea in general, but I think it's a very bad idea to implement something just for one search tool, as it's confusing for the end users if something is available with ag, but not with other tools. Do the other tools rely only on shell globbing for filtering?

@taquangtrung
Copy link
Contributor Author

@bbatsov: Thanks for the suggestion! I agree on that point.

I added the support for file extension filtering for all searchers now.

I also tested all of them manually in my Linux machine, and they all work well.

projectile.el Outdated Show resolved Hide resolved
@@ -4398,13 +4454,20 @@ to run the replacement."
(file-name-as-directory
(read-directory-name "Replace in directory: "))
(projectile-acquire-root)))
(file-ext (if arg
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps instead of reusing the other arg you want to use double prefix arg or something like this here? At any rate you'll also have to update the command doc, otherwise that would seem weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that using the double prefix arg is generally not natural: the user needs to pause to think about whether to use C-u or C-u C-u.

Instead, I revise the code to print a more suggestive message saying that if the users just need to enter an empty string to select all file extensions:

(file-ext (if arg
                       (if (fboundp #'helm-grep-get-file-extensions)
                           (car (helm-grep-get-file-extensions (list directory)))
                         (read-string
                          (projectile-prepend-project-name
                           "With file extension (empty string means all files): ")))
                     nil))

How do you think about that?

@bbatsov
Copy link
Owner

bbatsov commented May 31, 2022

The changes look reasonable to me, but you'll also have to mention them somewhere in the online docs, otherwise it's unlikely people will find this behavior.

And the tests seem to be failing now, so this has to be fixed.

@taquangtrung
Copy link
Contributor Author

The changes look reasonable to me, but you'll also have to mention them somewhere in the online docs, otherwise it's unlikely people will find this behavior.

And the tests seem to be failing now, so this has to be fixed.

Sorry, it was my mistake in the previous commit that I accidentally commented out a line of code, which lead to the failed tests.

Can you run the GitHub action again?

I also updated the CHANGELOG file. Where can I update the online documentation?

@bbatsov
Copy link
Owner

bbatsov commented Jun 14, 2022

I also updated the CHANGELOG file. Where can I update the online documentation?

It's generated from the .adoc files in the docs folder.

@bbatsov
Copy link
Owner

bbatsov commented Aug 26, 2022

Ping :)

@taquangtrung
Copy link
Contributor Author

@bbatsov: sorry for the long delay! I just updated the documentation.

@bbatsov
Copy link
Owner

bbatsov commented Aug 27, 2022

@taquangtrung It seems the reference to helm-grep-get-file-extensions is breaking the CI. You might have to declare it or something like this.

@taquangtrung
Copy link
Contributor Author

Thanks! I just updated it.

@bbatsov bbatsov merged commit 20aa2ad into bbatsov:master Aug 28, 2022
@bbatsov
Copy link
Owner

bbatsov commented Aug 28, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants