PyonFX
PyonFX copied to clipboard
Typing
This PR is aiming to fix some typehint and logic errors. There are still a lot of problems reported by mypy and pylance though.
TODO that would be nice:
- Make an abstract super class for Line, Word and Char since they share a lot of attributes.
- Use
isinstanceinstead oftype - Dissociate multiple return types. It's annoying for the user to know what a type a function returns unless they run the script. But it goes against the purpose of typehinting -> knowing type and prevents errors before running a script.
I would like to know your opinion about this before continuying to make new commits.
In https://github.com/CoffeeStraw/PyonFX/pull/33/commits/f509d84ce1ecc6d1aaae63b9cbdde6038488a41b an int is a float *
- Dissociate multiple return types. It's annoying for the user to know what a type a function returns unless they run the script. But it goes against the purpose of typehinting -> knowing type and prevents errors before running a script.
- Can you provide an example?
Something like that:
AssText = Union[Line, Word, Syllable, Char]
class Convert:
def __init__(self, x: Union[int, float, str, Tuple[float, ...], AssText, Shape]) -> None:
...
def to_assts() -> str:
...
def to_timems() -> int:
...
...
Using as following:
alpha = Convert(255).to_alpha_ass()
Additional arguments are added to the corresponding method
- Dissociate multiple return types. It's annoying for the user to know what a type a function returns unless they run the script. But it goes against the purpose of typehinting -> knowing type and prevents errors before running a script.
- Can you provide an example?
Something like that:
AssText = Union[Line, Word, Syllable, Char] class Convert: def __init__(self, x: Union[int, float, str, Tuple[float, ...], AssText, Shape]) -> None: ... def to_assts() -> str: ... def to_timems() -> int: ... ...Using as following:
alpha = Convert(255).to_alpha_ass()Additional arguments are added to the corresponding method
I can't really see the gain from that. There is no need to have a "Convert object" as we did for the Shape class, as we don't need to keep instance's values nor it is generally usefull to concatenate conversions.
I would stick with the actual implementation.
Sorry for the delay but I was a bit busy. I don't know why the tests are failing on Ubuntu. Maybe it's related to the font...
Maybe it's related to the font...
Correct. The reason is that installing the font failed.
Installing font from https://mirrors.tuna.tsinghua.edu.cn/osdn/mix-mplus-ipa/58720/migu-1p-20130430.zip
Downloading font file from https://mirrors.tuna.tsinghua.edu.cn/osdn/mix-mplus-ipa/58720/migu-1p-20130430.zip
HTTP request resulted in status 403
Detected content type: text/plain; charset=utf-8
not a font: migu-1p-20130430.zip
Installed 0 fonts
A workaround would be:
wget https://mirrors.tuna.tsinghua.edu.cn/osdn/mix-mplus-ipa/58720/migu-1p-20130430.zip
$HOME/.local/bin/font-install migu-1p-20130430.zip
Also, it might be worth to notice: https://golang.org/doc/go-get-install-deprecation
@Funami580 if we got HTTP 403 maybe it was a temporary error. By the way, updating go get with go install should be done for further compatibility.
We're getting closer!
- What about abstract classes?
- While you did well in changing repr to str, we might need to add a repr function, to have an additional unambiguous representation of the objects (as discussed in https://stackoverflow.com/questions/1436703/what-is-the-difference-between-str-and-repr);
Finally, after everything is done, we need to remember to check for generated documentation's correctness.
I think abstract classes should be done in a distinct PR
I've no idea right now of a "good" repr method. What's yours?
If by passing eval(repr(line)) you want to expect Line, an init has to be specify in ass text classes then
I think abstract classes should be done in a distinct PR
I've no idea right now of a "good" repr method. What's yours? If by passing eval(repr(line)) you want to expect Line, an init has to be specify in ass text classes then
Yeah, probably better to talk about that in a distinct PR.