Wilfred / ag.el

An Emacs frontend to The Silver Searcher
http://agel.readthedocs.org/en/latest/
525 stars 61 forks source link

Make ag.el default behavior consistent with ag's #110

Closed mernst closed 8 years ago

mernst commented 8 years ago

It is confusing to users that the default for ag is regexp search, but the default for ag.el is literal search. This pull request makes the two interfaces consistent.

A user can set a configuration option to make ag.el default to literal search: (setq ag-regexp-default nil)

Wilfred commented 8 years ago

Sorry, I don't think this is worthwhile.

ag.el provides two explicit commands, so it's unambiguous. I think users generally want the ability to use both, so a customisable setting is less convenient than separate commands.

ag.el currently has a consistent set of literal search commands: ag, ag-project, ag-dired, ag-project-dired, with a regexp version of each command. This PR breaks that consistency.

I'm also reluctant to change the default behaviour of the core command. I think that's surprising and would break downstream users, such as projectile-ag.

I know docs aren't great and new users may be surprised by ag-regexp using PCRE regexps rather than elisp regexps. Did use of a literal surprise you? Could we make this clearer?

mernst commented 8 years ago

Thanks for your reply.

ag.el provides two explicit commands, so it's unambiguous. I think users generally want the ability to use both, so a customisable setting is less convenient than separate commands.

The pull request retains this feature, so I don't think this is a reason to reject the pull request.

In the pull request, the two explicit commands are named ag-literal and ag-regexp, rather than being named ag and ag-regexp. In the pull request, the ag command is made to be customizable as literal or regexp, without affecting the other two.

I find the current naming confusing, especially since the ag Emacs command behaves differently than the ag command-line program. I feel that explicitly naming each version as *-literal or *-regexp will improve comprehensibility and is more in line with Emacs naming standards.

I'm also reluctant to change the default behaviour of the core command. I think that's surprising and would break downstream users, such as projectile-ag.

That is a reasonable concern, and I structured the pull request to accommodate it.

The pull request is two commits. If you accept just the first commit, then the user-visible behavior is unchanged, but users can set variable 'ag-regexp-defaultto get behavior that is consistent with the command-lineag`.

ag.el currently has a consistent set of literal search commands: ag, ag-project, ag-dired, ag-project-dired, with a regexp version of each command. This PR breaks that consistency.

Good point. Thanks for pointing that out. It would be good to make a similar change to what is in the pull request: Rename each ag-* version to ag-literal-* for clarity, and then re-introduce a new ag version whose behavior is controlled by variable ag-regexp-default.

I know docs aren't great and new users may be surprised by ag-regexp using PCRE regexps rather than elisp regexps. Did use of a literal surprise you? Could we make this clearer?

The undocumented use of a literal shocked me. I made serious mistakes in a few tasks. After I discovered my mistakes, I almost abandoned ag as buggy before realizing that the problem was in the Emacs interface.

The pull request changes the documentation string for ag from "given search STRING" to "given literal STRING". Even this minor improvement would have helped. However, customizability as provided in the pull request would provide better consistency with the command-line tool and would support users such as me who usually want to search for a regexp.

Thanks.

Wilfred commented 8 years ago

Thanks, I've cherry-picked 0a142d4 and improved docstrings in 76bded4 to clarify that we're using literals. I'm afraid I prefer the consistent ag-foo ag-foo-regexp command structure currently in place.