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 function/decorator checks for variable-format/resolution clips #37

Conversation

OrangeChannel
Copy link
Contributor

@OrangeChannel OrangeChannel commented Jun 4, 2020

This prevents us having to write the same ValueError for every function that requires a clip to have a format because it tries to access the clip.format's attributes.

vsutil/__init__.py Outdated Show resolved Hide resolved
@OrangeChannel OrangeChannel requested a review from stuxcrystal Jun 4, 2020
@OrangeChannel OrangeChannel changed the title Add private function to check if a clip is variable-format Add function to check if a clip is variable-format Jun 4, 2020
vsutil/__init__.py Outdated Show resolved Hide resolved
vsutil/__init__.py Outdated Show resolved Hide resolved
vsutil/__init__.py Outdated Show resolved Hide resolved
vsutil/__init__.py Outdated Show resolved Hide resolved
vsutil/__init__.py Outdated Show resolved Hide resolved
@tomato39
Copy link

tomato39 commented Jun 5, 2020

Is there a way to get mypy to recognize this? Right now it doesn't seem smart enough to follow the None check through the decorator.

Edit: potentially something like

class ConstantFormatNode(vs.VideoNode):
    format: vs.Format

def disallow_variable_format(function: Callable[..., R]) -> Callable[..., R]:
    """
    Function decorator that raises an exception if the input clip has a variable format.
    """
    def _check(clip: vs.VideoNode, *args: Any, **kwargs: Any) -> R:
        if is_variable(clip, format=True):
            raise ValueError('Variable-format clips not supported.')
        clip.__class__ = ConstantFormatNode
        return function(clip, *args, **kwargs)
    return _check

and making the unwrapped functions take ConstantFormatNode

@stuxcrystal
Copy link
Contributor

stuxcrystal commented Jun 5, 2020

@heicrd yes. typing.cast

But it seems that all type-checkers hate Decorators and Descriptors.

@OrangeChannel
Copy link
Contributor Author

OrangeChannel commented Jun 5, 2020

This can all be squashed into one commit with the PR title and reference #.

@OrangeChannel OrangeChannel changed the title Add function to check if a clip is variable-format Add function/decorator checks for variable-format/resolution clips Jun 5, 2020
@OrangeChannel
Copy link
Contributor Author

OrangeChannel commented Jun 5, 2020

Should we bump the version number in this update? This technically changes the exposed API by adding three new functions.
I don't know how git-tagging works for PRs or forks, I'd assume pushing tags directly to upstream/master is the better or only way to do it.

I recommend tagging ee49d9d as 0.3.0,
(and af4dd0a as 0.3.1 is we want to be specific), and whatever this squashed commit becomes as 0.4.0.

vsutil/__init__.py Outdated Show resolved Hide resolved
vsutil/__init__.py Outdated Show resolved Hide resolved
@OrangeChannel OrangeChannel requested a review from stuxcrystal Jun 6, 2020
@OrangeChannel
Copy link
Contributor Author

OrangeChannel commented Jun 8, 2020

So, I've completely satisfied mypy's issues on vsutil's side here: https://www.diffchecker.com/tGKtXhE5
Not sure if this fits in this PR, or is even wanted but figured I'd show @stuxcrystal @heicrd.

@kageru
Copy link
Member

kageru commented Jun 8, 2020

So, I've completely satisfied mypy's issues on vsutil's side here: https://www.diffchecker.com/tGKtXhE5
Not sure if this fits in this PR, or is even wanted but figured I'd show @stuxcrystal @heicrd.

I’m against doing that. Having the casts inside the decorators is fine, but adding them everywhere just to make a dysfunctional typechecker happy is not what I want. We shouldn’t make code more complicated just to please the IDE or some third-party tool.
Having decorators at all is already a level of complexity that might scare away potential contributors (most VS users don’t know much about Python), but I think they’re quite self-explanatory in this case, so I’m fine with that.

In general, we might consider making some of these functions work with variable formats (e.g. I don’t see a reason why depth shouldn’t work on a variable-depth input), but that’s out of scope here.

As for the tags, I’ve tagged the commit you suggested as 0.3.0, and I agree with tagging this as 0.4.0. I’ll do that after merge.
The decorators are part of the public API, and I believe they can be useful to other script and library authors, so I’d definitely consider that a new feature, and thus a minor version bump.
We technically also change the existing API, but in a very minor way (i.e. always raising ValueErrors, where previously an unintended AttributeError could have occured), but I don’t think that matters in practive due to how Vapoursynth scripts are usually written.

@tomato39
Copy link

tomato39 commented Jun 8, 2020

Yeah you can just cast inside the decorator and change the function signature to def fun(clip: _ConstantFormatVideoNode, ...) etc without having to use casts anywhere.

Having decorators at all is already a level of complexity that might scare away potential contributors

vsutil is already beyond this point with using typehints and unit tests in the first place imo. i don't think it's a good idea either way to try and draw contributors who have actually zero programming experience

vsutil/__init__.py Show resolved Hide resolved
@OrangeChannel
Copy link
Contributor Author

OrangeChannel commented Jun 8, 2020

I’ll make a more coherent response later but wanted to say that casting inside the decorator doesn’t solve mypy’s problem. It also creates another problem where another script calling get_subsampling(clip_VideoNode) is an error because isinstance(clip_VideoNode, _ConstantFormatNode) is False. mypy isn’t smart enough to know what to do with a changed signature via the decorator and casting inside the decorator means losing the ability to use the decorated functions with normal VideoNodes.

@kageru
Copy link
Member

kageru commented Jun 23, 2020

I’ll make a more coherent response later

@OrangeChannel I’m still kind of waiting for that.
Are there any other changes you still want to make here or can we merge this?

@OrangeChannel
Copy link
Contributor Author

OrangeChannel commented Jun 23, 2020

Oh I think I just explained the problem with typechecking the decorators in Light’s discord. There’s not much we can do here that wouldn’t be super over-engineered so as far as I’m concerned, the PR is ready to merge.

@OrangeChannel
Copy link
Contributor Author

OrangeChannel commented Jun 23, 2020

There’s a few open issues in mypy’s repo that might address this stuff in the future but for now the casting and stuff isn’t worth it.

@kageru kageru merged commit 32292f0 into Irrational-Encoding-Wizardry:master Jun 23, 2020
@OrangeChannel OrangeChannel mentioned this pull request Jun 29, 2020
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.

None yet

5 participants