Irrational-Encoding-Wizardry / vsutil

A collection of general purpose Vapoursynth functions to be reused in modules and scripts
MIT License
28 stars 7 forks source link

Split up the __init__.py file into multiple files. #38

Closed stuxcrystal closed 4 years ago

stuxcrystal commented 4 years ago

The reality is: We have validation functions, all in one file, can we split the module up?

OrangeChannel commented 4 years ago

How do we want to do this while keeping backwards compatibility? I assume we don't want to have to call plane from vsutil.util.plane so we should just import them into __init__.py (using an __all__ in each submodule and from .util import *), right? Do we want to discourage people from being able to call vsutil.util.plane?

Once we split the current file up, I can start working on the Sphinx documentation/GitHub actions to auto-build the docs to this repo's GitHub pages.

stuxcrystal commented 4 years ago

Well. I would not discourage people from using vsutil.util.plane

I'd go the route multiprocessing does:

from vsutil import util, ...

from vsutil.util.plane import *
...

__all__ = [*util.__all__, ...]
OrangeChannel commented 4 years ago

So how does a structure like this sound:

vsutil
\
  __init__.py
\
  util.py
    Dither
    Range
    _readable_enums()
    _resolve_enum()
    _should_dither()
    disallow_variable_format()
    disallow_variable_resolution()
    fallback()
\
  misc.py
    get_w()
    is_image()
    iterate()
\
  clips.py
    depth()
    frame2clip()
    get_y()
    insert_clip()
    join()
    plane()
    split()
\
  info.py
    get_depth()
    get_plane_size()
    get_subsampling()
FichteFoll commented 4 years ago

Maybe move the Dither, Range and other type-related stuff (the _ functions) to types.py?

kageru commented 4 years ago

I’d like to re-export all functions from the main module. That way, we’d have more structure for the maintainers without making it harder for users by removing their ability to from vsutil import plane (plus the aforementioned backward compatibility)

I’m also against a misc package because that kind of catch-all can easily grow and become very hard to properly separate (and there will come a point where we will want to split it up). get_w and is_image could be in info.py.

Having a util submodule in vs*util* seems very odd to me. Do we have other suggestions for naming this?

stuxcrystal commented 4 years ago
vsutil
\
  __init__.py
\
  types.py
    Dither
    Range
    _readable_enums()
    _resolve_enum()
    _should_dither()

\
  func.py
    disallow_variable_format()
    disallow_variable_resolution()
    fallback()
    iterate()

\
  path.py
    is_image()
    (is_video())     # <-- Maybe so this file doesn't look as useless
    (is_audio())     # <-- Once Audio-Support lands in VapourSynth

\
  clips.py
    depth()
    frame2clip()
    insert_clip()
    join()
    plane()
    split()

\
  info.py
    get_w()
    get_y()
    get_depth()
    get_plane_size()
    get_subsampling()

I personally hate misc.py as it is in my experience a chaos-magnet. But it util.py has the same problem imo. By carefully moving functions and introducing new categories, I came up with one function left that defies any proper categorization: is_image. This can be changed by adding companion-methods like is_video and (later, once VS-APIv4 comes out is_audio)

OrangeChannel commented 4 years ago

get_y() should be in clips.py since it returns a clip and not information about an existing clip. Will look at the rest and see if we might have any looping imports later.

kageru commented 4 years ago

I personally hate misc.py as it is in my experience a chaos-magnet. But it util.py has the same problem imo.

Are you sure the same won’t happen with func.py? The name feels rather generic as well., but I don’t have a better solution either. Either way, we should make sure (now and in later PRs) that func.py (or whatever the final name is) doesn’t import any other files. Everything can depend on those decorators/helpers, but they shouldn’t depend on anything. Otherwise cycles will become unavoidable.

I think is_image could be considered info if we don’t want a one-function module now. is_video doesn’t sound that useful, or at least I can’t come up with a good use case for it.

stuxcrystal commented 4 years ago

Just looking at the function name, every function called get_* returns something different than clip. Except get_y. I think the function get_y is badly named.

kageru commented 4 years ago

Well, it returns the Y plane, so I think the name is quite descriptive, and at this point, renaming it would probably break lots of scripts.

stuxcrystal commented 4 years ago

[kageru] if we don’t want a one-function module now. [OrangeChannel] Do we want to discourage people from being able to call vsutil.util.plane [kageru] and at this point, renaming it would probably break lots of scripts.

I think this discussions open a very important policy-question in general that have not been sufficiently answered: What actually is our policy for backwards compatibility?

We have a 0.x-version. Therefore technically, according to semver, we are in our right to just break compatibility. There will be a time where we want to deprecate stuff. But how do we deprecate stuff? Deprecating stuff is hard, as we have seen in the vapoursynth-module.

FichteFoll commented 4 years ago

For the record, I personally think it is fine to have some definitions in the package's main module, i.e. __init__.py, if no other categorization seems to fit.

I would also be okay an internal-only util module assuming it does not contain things that should be exported. func.py from the latest suggestion serves the same purpose under a different name. To emphasize, you could prefix with an underscore.

kageru commented 4 years ago

we are in our right to just break compatibility

That is correct, but I don’t want to exercise that right unnecessarily, and renaming a (probably very frequently used) function for no reason other than it being confusing at first, especially when we’re now splitting the module into multiple files which would/should resolve that confusion, doesn’t feel right. The more frequently we break our API, the more users will want to avoid updating vsutil or depending on it altogether. I understand your point, but I don’t think renaming is the reasonable reaction here.

Deprecation is pretty difficult in Vapoursynth in general because I don’t think you have a good way of communicating to your user that something is deprecated.

OrangeChannel commented 4 years ago

@stuxcrystal I tried to implement what you suggested in the easiest way for future contributors/maintainers to add new sub-modules, or functions to current files. Only requires adding a new from .xxx import * to __init__.py and adding the submodule name to _mods list. New functions can simply be added to each submodule's __all__s.