add rg-dwim-regexp #28

zonotope · 2017-12-25T05:57:37Z

Add a new function, rg-dwim-regexp to search for a user provided pattern using the default type alias. rg-dwim-regexp is like a cross between rg-project and rg-dwim in that it skips the files confirmation but still lets you enter your own pattern.

coveralls · 2017-12-25T06:04:10Z

Coverage decreased (-0.5%) to 77.957% when pulling e0ba1ba on zonotope:master into 6898409 on dajva:master.

coveralls · 2017-12-25T06:04:10Z

Coverage decreased (-0.5%) to 77.957% when pulling e0ba1ba on zonotope:master into 6898409 on dajva:master.

coveralls · 2017-12-25T06:04:10Z

Coverage decreased (-0.5%) to 77.957% when pulling e0ba1ba on zonotope:master into 6898409 on dajva:master.

coveralls · 2017-12-25T21:30:27Z

Coverage decreased (-0.5%) to 77.957% when pulling 9ca4eda on zonotope:master into 6898409 on dajva:master.

coveralls · 2017-12-25T21:30:27Z

Coverage decreased (-0.5%) to 77.957% when pulling 9ca4eda on zonotope:master into 6898409 on dajva:master.

coveralls · 2017-12-25T21:30:27Z

Coverage decreased (-0.5%) to 77.957% when pulling 9ca4eda on zonotope:master into 6898409 on dajva:master.

coveralls · 2017-12-25T21:30:27Z

Coverage decreased (-0.5%) to 77.957% when pulling 9ca4eda on zonotope:master into 6898409 on dajva:master.

coveralls · 2018-01-07T03:28:47Z

Coverage increased (+4.3%) to 82.707% when pulling 64d7bf0 on zonotope:master into 6898409 on dajva:master.

coveralls · 2018-01-08T02:06:05Z

Coverage increased (+4.0%) to 82.368% when pulling 9a24e3a on zonotope:master into 6898409 on dajva:master.

coveralls · 2018-01-08T02:06:05Z

Coverage increased (+4.0%) to 82.368% when pulling 9a24e3a on zonotope:master into 6898409 on dajva:master.

dajva

Sorry for my slow response on this. Looking good in general. I will be quicker from now on.

dajva · 2018-01-11T16:24:41Z

rg.el

@@ -409,12 +409,13 @@ If LITERAL is non nil prompt for literal pattern."
                                   nil default-directory t)))
    (list pattern files dir)))

-(defun rg-read-files (pattern)
+(defun rg-read-files (&optional pattern)


The pattern isn't really used from anywhere now. That's actually unfortunate since it's quite useful to have that in the prompt, especially in the functions that extract the pattern automatically. I think the code could be arranged to not use the interactive macro for user interactive input and reorder things such that pattern is extracted first.

I don't think this is super important to spend much time on so I am fine if you remove the pattern argument and cleanup the code below according to that.

@dajva I think the reason I made this change was that I couldn't figure out a way to guarantee that the pattern was always set before the file alias was set without changing the function signatures in some cases, and I wanted to keep the existing api the same. I think if we stopped using the interactive macro to set the pattern, then we'd have to change the way the functions get called in the tests (and this could also possibly impact some user code that relies on the existing functions).

Do you know a way around this?

I've had problems with this as well. I kind of regret the choice of using interactive for the input but I guess I'll have to live with it for now. This way of incuding the pattern in the prompt is actually a legacy from when I started this based on grep.el and I am not sure I would have done the same choice now, although it's kind of useful sometimes.

So, just ignore this. If you want to clean up the defun do that, otherwise I can fix later on.

dajva · 2018-01-11T16:31:11Z

rg.el

@@ -1007,67 +1008,124 @@ prefix is not supplied `rg-keymap-prefix' is used."
    (message "Global key bindings for `rg' enabled with prefix: %s"
             (edmacro-format-keys prefix))))

+(defun rg-set-search-defaults (args)
+  (if (not (plist-get args :pattern)) (plist-put args :pattern 'regexp)


Formatting: Put the "then" clause on next line.

dajva · 2018-01-11T16:32:17Z

rg.el

-constructed shell command line before it is executed."
-  (interactive
-   (progn
-     (append (rg-read-input 'literal)


rg-read-input is unused now so please remove it.

dajva · 2018-01-11T17:22:20Z

rg.el

+         (rg-run pattern files dir literal confirm)))))
+
+;;;###autoload
+(rg-define-search rg-project


Can you readd all the removed docstrings to the new entries, using the macro?

dajva · 2018-01-11T17:26:43Z

rg.el

+
+;;;###autoload
+(defmacro rg-define-search (name &rest args)
+  (let* ((body (macroexp-parse-body args))


You can't use macroexp-parse-body unfortunately, since this needs to work all the way back to emacs 24.3. :-(

yeah, that's a bummer. i'll probably just end up copying the definition of that function as a helper here then.

Sure, that's fine.

dajva · 2018-01-11T18:06:07Z

rg.el

+    iargs))
+
+;;;###autoload
+(defmacro rg-define-search (name &rest args)


The macro itself needs docs

dajva · 2018-01-11T18:07:20Z

rg.el

+(defun rg-search-parse-local-bindings (search-cfg)
+  (let* ((binding-list `((confirm ,(plist-get search-cfg :confirm))))
+         (pattern-opt (plist-get search-cfg :pattern))
+         (literal (or (eq pattern-opt 'literal)


Can you split the :pattern key into :pattern = { 'point } and :type = { 'literal, 'regexp } ? I'd like to keep the interface flexible and splitting this will ease extending this further on.

I was mid-cleanup when you reviewed this. I had already split the :pattern key in to :query = {'ask 'point} and :format = {'regexp 'literal}. I can change these names to what you suggest if you prefer those though.

What you did is perfect.

dajva · 2018-01-11T18:07:44Z

rg.el

+    (setq binding-list (append binding-list `((literal ,literal))))
+
+    ;; pattern binding
+    (when (not (or (eq pattern-opt 'literal)


Use unless instead of (when (not. Can you change all such occurances in this pr?

dajva · 2018-01-11T18:22:19Z

rg.el

+    (rg-dwim-project-dir)))
+
+;;;###autoload
+(rg-define-search rg-dwim-regexp


Can you change the name of this? I want the dwim entries to be "no-hands", i.e. you shouldn't have any manual input for those. As for a name. rg-project- prefix seems approriate.

i decided to remove this function all together. i agree with you that the first name was inappropriate, but i couldn't think of another good name either. more importantly, i don't think this function is necessary in the library anymore since custom search functions are so easy to define with the rg-define-search macro anyway.

dajva · 2018-01-11T19:38:10Z

rg.el

+    binding-list))
+
+(defun rg-search-parse-interactive-pattern-arg (search-cfg)
+  (let* ((opt (plist-get search-cfg :pattern))


Docstrings for all new defuns. Need that to pass ci tests.

coveralls · 2018-01-14T15:49:10Z

Coverage increased (+6.1%) to 84.541% when pulling a515fcf on zonotope:master into 6898409 on dajva:master.

coveralls · 2018-01-14T15:49:10Z

Coverage increased (+6.1%) to 84.541% when pulling a515fcf on zonotope:master into 6898409 on dajva:master.

coveralls · 2018-01-14T15:49:10Z

Coverage increased (+6.1%) to 84.541% when pulling a515fcf on zonotope:master into 6898409 on dajva:master.

zonotope · 2018-01-14T17:08:16Z

@dajva I'm not sure what to make of the CI failure. It looks like the style linter doesn't load the definitions of any of the functions referenced in the rg-define-search macro. I can't tell what the problem could be from looking through style-check.el, all the tests pass, and I can successfully load rg.el, define a custom search in an emacs instance, and the functions work as I expect. Do you know what the problem could be?

dajva · 2018-01-14T19:21:36Z

rg.el

@@ -409,12 +409,13 @@ If LITERAL is non nil prompt for literal pattern."
                                   nil default-directory t)))
    (list pattern files dir)))

-(defun rg-read-files (pattern)
+(defun rg-read-files (&optional pattern)


I've had problems with this as well. I kind of regret the choice of using interactive for the input but I guess I'll have to live with it for now. This way of incuding the pattern in the prompt is actually a legacy from when I started this based on grep.el and I am not sure I would have done the same choice now, although it's kind of useful sometimes.

So, just ignore this. If you want to clean up the defun do that, otherwise I can fix later on.

dajva · 2018-01-14T19:33:09Z

rg.el

-         (docstring (car body))
+  "Define an rg search functions named NAME.
+ARGS is a search specification with `:query' (point or ask),
+`:format' (literal or regexp), `files' (rg or custom file alias),


The format of the keys and symbolx is inconsistent. Here you use the `:key' form for keys and no quote for the symbols. Above you just write :key and quoted symbols. I am not entirely sure what's common in this area but
I think the backquote-quote format is only for symbols that can be looked up, like vars and defuns etc so that links will be created in the documentation.
For the symbols used as values for the keys I used uppercase in rg-ignore-case. I am not entirely sure what should be used here. If you don't have more insights into this could you use uppercase as I used before to keep it consistent?

yeah, I don't know either. I ended up going with uppercase for the symbal definitions, and getting rid of the the backquote-quote for keywords. One thing I don't like about the uppercase form is that it isn't literally what the user should pass when calling the function/macro, but the values have to get differentiated from normal text somehow, and that wouldn't be the literal form either.

Yeah, you might be right that the uppercase is the wrong call. Let's keep this for now and I will see if I can find something about how others do it. Quoted form for the symbols seems to be the easiest one to interpret.

dajva · 2018-01-14T19:49:17Z

Regarding the CI failure I think you need to wrap the defuns used from the macro in eval-when-compile to make those available during compilation (which is what happens in the style-check). At least that works locally for me.

coveralls · 2018-01-15T01:48:17Z

Coverage increased (+4.7%) to 83.146% when pulling c558a14 on zonotope:master into 6898409 on dajva:master.

coveralls · 2018-01-15T01:48:17Z

Coverage increased (+4.7%) to 83.146% when pulling c558a14 on zonotope:master into 6898409 on dajva:master.

dajva · 2018-01-15T20:34:15Z

Thanks a lot for your effort on this. Very nice implementation and well structured code.
This will greatly improve the usability of this package.

zonotope · 2018-01-16T00:50:02Z

thank you for writing this package in the first place! i'm glad i could help.

add rg-dwim-regexp

e0ba1ba

zonotope mentioned this pull request Dec 25, 2017

Skip files confirmation? #27

Closed

zonotope closed this Dec 25, 2017

zonotope added 2 commits December 25, 2017 15:51

add docstrings

df18cec

add test for rg-dwim-regexp

9ca4eda

zonotope reopened this Dec 25, 2017

add a macro to define search configurations

64d7bf0

allow rg-define-search to take an optional docstring

9a24e3a

add some docstrings.

7a1a875

dajva requested changes Jan 11, 2018

View reviewed changes

zonotope added 2 commits January 13, 2018 23:55

split pattern arg; docstrings; cleanup

a3b91df

set confirm option in local bindings

a515fcf

dajva reviewed Jan 14, 2018

View reviewed changes

zonotope added 2 commits January 14, 2018 20:26

evaluate function definitions during compilation

ba37fe7

make the documentation consistent.

c558a14

dajva approved these changes Jan 15, 2018

View reviewed changes

dajva merged commit 2a3e6ac into dajva:master Jan 15, 2018

add rg-dwim-regexp #28

add rg-dwim-regexp #28

Conversation

zonotope commented Dec 25, 2017

coveralls commented Dec 25, 2017 • edited Loading

coveralls commented Dec 25, 2017

coveralls commented Dec 25, 2017

coveralls commented Dec 25, 2017 • edited Loading

coveralls commented Dec 25, 2017 • edited Loading

coveralls commented Dec 25, 2017

coveralls commented Dec 25, 2017

coveralls commented Jan 7, 2018 • edited Loading

coveralls commented Jan 8, 2018

coveralls commented Jan 8, 2018

dajva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonotope Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dajva Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonotope Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 14, 2018 • edited Loading

coveralls commented Jan 14, 2018

coveralls commented Jan 14, 2018

zonotope commented Jan 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dajva commented Jan 14, 2018

coveralls commented Jan 15, 2018 • edited Loading

coveralls commented Jan 15, 2018

dajva commented Jan 15, 2018

zonotope commented Jan 16, 2018

coveralls commented Dec 25, 2017 •

edited

Loading

coveralls commented Dec 25, 2017 •

edited

Loading

coveralls commented Dec 25, 2017 •

edited

Loading

coveralls commented Jan 7, 2018 •

edited

Loading

zonotope Jan 13, 2018 •

edited

Loading

dajva Jan 11, 2018 •

edited

Loading

zonotope Jan 14, 2018 •

edited

Loading

coveralls commented Jan 14, 2018 •

edited

Loading

coveralls commented Jan 15, 2018 •

edited

Loading