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

Switch to use transient.el #26

Merged
merged 5 commits into from
Aug 4, 2020
Merged

Conversation

pkryger
Copy link
Contributor

@pkryger pkryger commented Aug 2, 2020

transient is a replacement to magit-popup and even magit has already switched to use transient.

This fixes #18.

The switch is pretty idiomatic, however there are a few differences.

The default layout is slightly different but I didn't try to replace it as the new one seems to match what magit is presenting these days. I think that the new layout resembles more what magit is presenting these days, but I don't have any scientific method for that. Happy to work on changing to original layout if this is desired.

The concept of :default-action seems to be gone, so I did not replace it.

The magit-with-pre-popup-buffer doesn't seem to have an obvious replacement, however the documentation for transient--original-buffer suggest that the transient popup is not changing the current buffer. I validated that with a check that the current-buffer returns expected pytest buffer while invoking python-pytest via python-pytest-popup.

python-pytest.el Outdated Show resolved Hide resolved
python-pytest.el Outdated Show resolved Hide resolved
README.rst Show resolved Hide resolved
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 is looking quite good already. i left a few comments; nothing major.

@pkryger
Copy link
Contributor Author

pkryger commented Aug 3, 2020

Thank you for review. Please let me know which of fixes to comments you'd like to see in before merging.

@wbolster
Copy link
Owner

wbolster commented Aug 3, 2020

i responded inline to most comments. happy to merge this after making a tiny adjustment.

i will experiment with shortening some names later. i'll ping you for feedback once i have a pr for it, if you don't mind.

i actually started a branch locally to switch to transient.el but stopped working on it two minutes later since i got distracted life happened.

and then all of a sudden someone else did all the work. 👍😄

@wbolster
Copy link
Owner

wbolster commented Aug 3, 2020

oh one more thing: defining python-pytest-arguments in init.el was a good way to set defaults before.

is there a clear replacement for it? if so, it may be worth adding to the readme.

@wbolster
Copy link
Owner

wbolster commented Aug 3, 2020

oh yet another thing, maybe use python-pytest-dispatch as the name of the function (and helpers). it seems to be the convention?

i guess an alias for the obsolete name (defsubst iirc, typing from phone) would help with upgrades.

but it's not required and i can do it later.

`transient` is a replacement to `magit-popup` and even `magit` has already
switched to use `transient`.

The switch is pretty idiomatic, however there are a few differences.

The default layout is slightly different but I didn't try to replace it as the
new one seems to match what `magit` is presenting these days.

The concept of `:default-action` seems to be gone, so it is not replaced.

The `magit-with-pre-popup-buffer` doesn't seem to have an obvious replacement,
however the documentation for `transient--original-buffer` suggest that the
`transient` is not changing the current buffer.
@pkryger
Copy link
Contributor Author

pkryger commented Aug 3, 2020

i'll ping you for feedback once i have a pr for it, if you don't mind.

Sure. Happy to help with it.

oh yet another thing, maybe use python-pytest-dispatch as the name of the function (and helpers). it seems to be the convention?

Let me see what I can do today.

is there a clear replacement for it? if so, it may be worth adding to the readme.

Perhaps there's some programmatic way, but I'm not aware of it. What I know is transient has a concept of saving your transient state as a default. Just C-x C-s after you selected all options and this will be the new default (persistent) or C-x s and it will persist in until emacs restart. I will update readme.

python-pytest.el Outdated Show resolved Hide resolved
python-pytest.el Outdated Show resolved Hide resolved
@wbolster
Copy link
Owner

wbolster commented Aug 4, 2020

nice, i see you did the rename to -dispatch, thanks!

python-pytest.el Outdated Show resolved Hide resolved
Copy link
Contributor Author

@pkryger pkryger left a comment

Choose a reason for hiding this comment

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

Will update with the removal of fmakeunbound/makeunbound

python-pytest.el Outdated Show resolved Hide resolved
python-pytest.el Outdated Show resolved Hide resolved
pkryger and others added 2 commits August 4, 2020 14:18
With transient, an `eval-buffer` is picking up new definition of
`python-pytest-dispatch` (the popup).
@wbolster wbolster changed the title Switch to use transient Switch to use transient.el Aug 4, 2020
@wbolster wbolster merged commit ee61741 into wbolster:master Aug 4, 2020
@wbolster
Copy link
Owner

wbolster commented Aug 4, 2020

merged, thanks a lot for doing all the hard/boring (choose yourself) work!

@pkryger
Copy link
Contributor Author

pkryger commented Aug 4, 2020

Pleasure. It was really nice to work with you!

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 wbolster mentioned this pull request Aug 4, 2020
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update from magit-popup to transient
2 participants