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

Library: Port 'Video' to Python #728

Merged
merged 1 commit into from
Nov 11, 2023
Merged

Library: Port 'Video' to Python #728

merged 1 commit into from
Nov 11, 2023

Conversation

UrtsiSantsi
Copy link
Contributor

No description provided.

video.set_file(Gio.File.new_for_uri(workbench.resolve("./workbench-video.mp4")))


def on_pressed(*_):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not very sure if this is the best way - it gets 4 parameters that we don't care about

Copy link
Contributor

Choose a reason for hiding this comment

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

*_ is a bit odd style, normally this is always called *args, but it's OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

But yeah this is kind of the way to go if you don't care about the signal's parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be consistent with code style 🙏

@theCapypara you can document Python code style in https://github.com/sonnyp/Workbench/wiki/Style-Guide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I change it to "args" or leave it as "_" to indicate that they are discarded?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know we already merged a few PRs with *_ but I would really prefer *args.

@sonnyp I'll do that later once the storm of demo PRs is done, I'll also see if we can add linting then, it would probably point this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about methods that have multiple discards like lambda _, __ should it become lambda *args? And what about lambda x, _, y, __?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the first case, yes. In the second you could do x, _a, y, _b if it's just one extra argument or also x, _a, y, *args in general.

Copy link
Contributor

@theCapypara theCapypara left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you! The only thing to note was the *_ naming, see inline comments. However we will deal with this when we add linting later.

@theCapypara theCapypara merged commit 43bb5d3 into main Nov 11, 2023
1 check passed
@theCapypara theCapypara deleted the python_video branch November 11, 2023 15:59
lw64 pushed a commit that referenced this pull request Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants