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 very verbose option #22

Merged
merged 6 commits into from
Jul 30, 2020
Merged

Add very verbose option #22

merged 6 commits into from
Jul 30, 2020

Conversation

r-zip
Copy link
Contributor

@r-zip r-zip commented Jul 22, 2020

Pytest has an option for higher verbosity, which will display the full diff for objects that fail equality assertion (-vv). In my work, I've found the flag to be indispensable for TDD. This PR adds the -vv flag as an option in the magit popup menu.

Copy link
Owner

@wbolster wbolster left a comment

Choose a reason for hiding this comment

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

thanks for your contribution. this seems useful indeed. i usually add it manually, or specify it via addopts in pytest.ini.

i think the implementation can be nicer though: instead of an extra flag, perhaps the -v flag could ‘rotate’ between nothing, --verbose, and --verbose --verbose instead. what do you think?

@wbolster wbolster self-assigned this Jul 22, 2020
@wbolster wbolster added the enhancement New feature or request label Jul 22, 2020
@r-zip
Copy link
Contributor Author

r-zip commented Jul 30, 2020

I've tried this (you can take a look at my best attempt in the most recent commit), but I don't think there is a way to do this within the magit-popup framework. I think we'd either need to submit a PR upstream for it, or hack around it (which might take a lot of effort for something so simple).

The issue is that we need to maintain some state, which I tried to do by setting the SWITCH argument to a 0-arity function that returns a string and manages a counter for the "verbosity level". It seemed in the docs that this was supported, but it doesn't work, so I must have misread something.

I've never used magit-popup extensively, so let me know if I'm missing anything obvious.

@wbolster
Copy link
Owner

same here, never used it extensively, and it's semi-deprecated in favour of transient.el. i can't say i like the implementation for the same reasons you mention, and perhaps it can be simplified a bit somehow, but i don't think it's worth it.

let's settle for good enough. thanks!

@wbolster wbolster merged commit 6d8538a into wbolster:master Jul 30, 2020
wbolster added a commit that referenced this pull request Jul 30, 2020
wbolster added a commit that referenced this pull request Jul 30, 2020
@wbolster
Copy link
Owner

you're right, custom lisp functions don't seem to work at all for ‘switches’ (e.g. -x) like they do for ‘options’ (e.g. =x).

the magit-invoke-popup-switch has no funcall, unlike magit-invoke-popup-option:

(defun magit-invoke-popup-switch (event)
  (interactive (list last-command-event))
  (--if-let (magit-popup-lookup event :switches)
      (progn
        (setf (magit-popup-event-use it)
              (not (magit-popup-event-use it)))
        (magit-refresh-popup-buffer))
    (user-error "%c isn't bound to any switch" event)))

(defun magit-invoke-popup-option (event)
  (interactive (list last-command-event))
  (--if-let (magit-popup-lookup event :options)
      (progn
        (if (magit-popup-event-use it)
            (setf (magit-popup-event-use it) nil)
          (let* ((arg (magit-popup-event-arg it))
                 (val (funcall
                       (magit-popup-event-fun it)
                       (concat arg (unless (string-match-p "=$" arg) ": "))
                       (magit-popup-event-val it))))
            (setf (magit-popup-event-use it) t)
            (setf (magit-popup-event-val it) val)))
        (magit-refresh-popup-buffer))
    (user-error "%c isn't bound to any option" event)))

@r-zip r-zip mentioned this pull request Jul 30, 2020
@wbolster
Copy link
Owner

fwiw, #24 was merged with the original simpler idea: a -w flag

wbolster added a commit that referenced this pull request Aug 4, 2020
Make the ‘-v’ option cycle between nothing, ‘--verbose’, and ‘--verbose
--verbose’. This was suggested before
(#22 (review))
but wasn't possible at the time of #22 because it seemed impossible due
to magit-popup.el limitations.

Now that the dispatch menu uses transient.el instead of magit-popup.el
(#18, #26), it can be implemented after all
wbolster added a commit that referenced this pull request Aug 4, 2020
Make the ‘-v’ option cycle between nothing, ‘--verbose’, and ‘--verbose
--verbose’. This was suggested before
(#22 (review))
but wasn't possible at the time of #22 because it seemed impossible due
to magit-popup.el limitations.

Now that the dispatch menu uses transient.el instead of magit-popup.el
(#18, #26), it can be implemented after all.
wbolster added a commit that referenced this pull request Aug 4, 2020
Make the ‘-v’ option cycle between nothing, ‘--verbose’, and ‘--verbose
--verbose’. This was suggested before
(#22 (review))
but wasn't possible at the time of #22 due to magit-popup.el limitations.

Now that the dispatch menu uses transient.el instead of magit-popup.el
(#18, #26), it can be implemented after all.
wbolster added a commit that referenced this pull request Aug 5, 2020
Make the ‘-v’ option cycle between nothing, ‘--verbose’, and ‘--verbose
--verbose’. This was suggested before
(#22 (review))
but wasn't possible at the time of #22 due to magit-popup.el limitations.

Now that the dispatch menu uses transient.el instead of magit-popup.el
(#18, #26), it can be implemented after all.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants