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

Feat: Actions #1349

Merged
merged 46 commits into from
Aug 1, 2023
Merged

Feat: Actions #1349

merged 46 commits into from
Aug 1, 2023

Conversation

benjamin-kirkbride
Copy link
Contributor

@benjamin-kirkbride benjamin-kirkbride commented Jul 7, 2023

First pass, here is how you would register an action that is only available for Markdown files against Paths:

register_path_action(
    name="foo",
    description="bar",
    callback=lambda: None,
    availability_callbacks=[filetype_availability(["Markdown"])],
)

I was going to have some dedicated arguments for things like filetype availability, whether or not you can use a directory or a file path, etc, but ultimately I think what makes the most sense is having a single callback that takes a list of availability_callbacks, idea being these callbacks will be executed in order, and if any return False, the action is not available. I think that creating helpers for this will be extremely flexible, composable, and dev friendly.

Thoughts on how it is so far?

Fixes #1341

ChatGPT sent me on so many wild goose chases, I tried:
- Protocol
- singledispatch
- overloads

and none of them worked. ChatGPT spun tales of how Mypy is incapable of detecting callables through partials, how it can't track duck typed functions, etc etc

literally for a 2 character change fml
@benjamin-kirkbride
Copy link
Contributor Author

Instead of having 3 register_*action commands, I could have boolean switches on a single register_action function. What are your thoughts?

Copy link
Owner

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

I like the design overall :)

porcupine/actions.py Outdated Show resolved Hide resolved
porcupine/actions.py Outdated Show resolved Hide resolved
porcupine/actions.py Outdated Show resolved Hide resolved
porcupine/actions.py Outdated Show resolved Hide resolved
porcupine/actions.py Outdated Show resolved Hide resolved
porcupine/actions.py Outdated Show resolved Hide resolved
@Akuli
Copy link
Owner

Akuli commented Jul 7, 2023

I think I want the 3 separate functions, at least for now.

@Akuli
Copy link
Owner

Akuli commented Jul 7, 2023

I spent literally 3 hours trying to figure this out

Feel free to ask me if I'm online :D

Copy link
Owner

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

Just a few nits.

porcupine/actions.py Outdated Show resolved Hide resolved
porcupine/actions.py Outdated Show resolved Hide resolved
porcupine/actions.py Outdated Show resolved Hide resolved
@benjamin-kirkbride
Copy link
Contributor Author

I think it's fair to call "actions.py: the action classes, the registry and the related functions" done now

Copy link
Owner

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

The last nit...

porcupine/menubar.py Outdated Show resolved Hide resolved
porcupine/menubar.py Outdated Show resolved Hide resolved
@benjamin-kirkbride benjamin-kirkbride mentioned this pull request Jul 12, 2023
6 tasks
@Akuli
Copy link
Owner

Akuli commented Jul 13, 2023

The discussion on this PR has become a bit tl;dr to follow. I will hopefully have time to look at this properly during the weekend :)

@benjamin-kirkbride
Copy link
Contributor Author

@Akuli any update on this? I think it's in a good state.

porcupine/actions.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@rdbende rdbende left a comment

Choose a reason for hiding this comment

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

Some nits, but otherwise LGTM

porcupine/menubar.py Outdated Show resolved Hide resolved
porcupine/actions.py Show resolved Hide resolved
porcupine/actions.py Outdated Show resolved Hide resolved
porcupine/menubar.py Outdated Show resolved Hide resolved
porcupine/menubar.py Outdated Show resolved Hide resolved
@benjamin-kirkbride
Copy link
Contributor Author

Only thing left at this point is docs IMO. Is it worth doing that before more stuff uses actions?

def set_enabled_based_on_tab(
path: str, callback: Callable[[tabs.Tab | None], bool]
) -> Callable[..., None]:
_menu_item_enabledness_callbacks: list[Callable[..., None]] = []
Copy link
Owner

Choose a reason for hiding this comment

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

Not super happy with this design, but I just want this merged.

@Akuli Akuli merged commit 9ff4be0 into Akuli:main Aug 1, 2023
19 checks passed
@benjamin-kirkbride benjamin-kirkbride deleted the actions_init branch August 1, 2023 22:11
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.

Actions - Initial Implementation
4 participants