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 the ability to run specs filtered by tags #217

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

Conversation

MoskitoHero
Copy link

This PR proposes to add the ability to run specs filtered by tag(s).

It adds four functions:

  • rspec-verify-tagged: only verify the tag(s) in the current file
  • rspec-verify-all-tagged: only verify the tag(s) in the whole suite
  • rspec-dired-verify-tagged: only verify the tag(s) in the current dired scope
  • rspec-dired-verify-single-tagged: only verify the tag(s) in the currently marked files

It also adds the corresponding key bindings

The functions prompt the user for one or more tags following the rspec syntax and appends them to the rspec command.

See https://rspec.info/features/3-12/rspec-core/command-line/tag/

This commit adds two functions: rspec-verify-tagged and rspec-verify-all-tagged, that prompt the user for one or more tags following the rspec syntax and appends them to the rspec command.

See https://rspec.info/features/3-12/rspec-core/command-line/tag/
@dgutov
Copy link
Collaborator

dgutov commented Mar 3, 2024

Hi!

Filtering by tags should indeed be a useful addition. Thanks.

Before I merge this, though, have you considered the following alterations:

  1. Instead of adding new commands, adding a prefix argument to the existing commands - so when called with C-u the command would read the list of tags to use. Or this it too slow for your intended usage?
  2. Tags-specific input history - so when you run the command multiple times you can quickly switch to the previous input, but limited to the previous entered tags, not just any strings that were read by previous commands. That can work through an explicit call to read-string inside the (interactive ...) form.
  3. If 1 is a no-go, and new commands are really needed, how about we put them on a separate submap, rather than having three new characters to memorize? Then rspec-verify-tagged would by default be on C-c , t v.

  This commit:
  - Renames functions from `-tagged` to `verify-tags-*` -- this avoids confusion in dired mode with dired `-tagged` functions, and is clearer too, imho
  - Saves previous tags to `rspec-tags-history`
  - Persists `rspec-tags-history` to savehist if `savehist-mode` is on
  - Adds `C-c , g` prefix for all tag function keymaps (`t` clashes with current keymap)
@MoskitoHero
Copy link
Author

Thanks for taking the time to review this, and thanks for your input, which I believe improves the PR. 🙏

I don't think I want to go for 1. I feel it is too slow indeed, when coding / running tests, I want commands to be straightforward and get out of the way, so I decided to go for 2.

So I have added a rspec-tags-history variable. Each new tag is added to the list, which is persisted in-between sessions if savehist-mode is on.

I also decided to rename functions from -tagged to verify-tags-* -- this avoids confusion in dired mode with dired -tagged functions, and is clearer too, IMHO.

I finally added the g submap (C-c , t clashes with rspec-toggle-spec-and-target) for all tagged functions.

"Run the specified spec, or the spec file for the current buffer, filtered by one or multiple tags."
(interactive)
(rspec--autosave-buffer-maybe)
(let* ((tags-string (completing-read-multiple "Select tags (separated by comma): " rspec-tags-history))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to use the history variable as the HIST argument in completing-read-multiple. It's the argument number 6. Doing that would automatically add the selected input to history as well (so rspec-add-tags-to-history won't be needed, I think).

I guess you could pass rspec-tags-history as TABLE and 'rspec-tags-history (quoted) as the history variable, so both saving to history works, and completion works as well.

"Run the `spec' rake task for the project of the current file, filtered by one or multiple tags."
(interactive)
(let* ((tags-string (completing-read-multiple "Select tags (separated by comma): " rspec-tags-history))
(tags-list (if (stringp tags-string) (split-string tags-string ",\\s-*") tags-string)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the docstring, completing-read-multiple always returns a list. So (stringp tags-string) should always be nil, and tags-string can be renamed to tags.

(let* ((tags-string (completing-read-multiple "Select tags (separated by comma): " rspec-tags-history))
(tags-list (if (stringp tags-string) (split-string tags-string ",\\s-*") tags-string)))
(rspec-add-tags-to-history tags-list)
(rspec-compile (dired-get-marked-files)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's a way to make this shorter: change the value of rspec-command-options locally (in the let form) and then call the "base" function. Like this:

(defun rspec-dired-verify-tags-single ()
  "Run marked specs or spec at point, filtered by one or multiple tags (works with directories too)."
  (interactive)
  (let* ((tags (completing-read-multiple "Select tags (separated by comma): " rspec-tags-history))
         (rspec-command-options (concat rspec-command-options
                                        " "
                                        (mapconcat (lambda (tag) (format " --tag %s" tag)) tags " "))))
    (rspec-dired-verify-single)))

Same with the other commands.

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