Feature type hinting

[maestro-1]

Initial add of types to the sequence class and the functions of transformation module
This is an initial commit for #118

The Sequence class has a parameter of no_wraps which is a keyword argument. By all indications, this is a boolean but the default parameter used was None. This can lead to confusion if not properly understood, so type hints have been included to specify it as a boolean, and the default parameter has been changed from None to False. They would run the same way, but False is more semantically accurate in this context.

The typing module is deprecated in more recent versions of python, but generics still need to be carried out using TypeVar until python 3.12. For backward compatibility using TypeVar is preferable.

[artemisart]

You can't replace no_wrap=None with False, None is used to default to self.no_wrap (but you should still be able to override a self.no_wrap=True with no_wrap=False).

[artemisart]

revert changes on no_wrap cf previous comment

[maestro-1]

You can't replace no_wrap=None with False, None is used to default to self.no_wrap (but you should still be able to override a self.no_wrap=True with no_wrap=False).

I am not sure I see the issue. None and False for all intents and purposes would amount to the same response when passed through a conditional. Like None, if it is false it should still default to self.no_wrap unless I am missing something, in which case please outline.
Making use of None for a field that should be otherwise boolean seems like a recipe for confusion. In fact reading through, one would have to track many wrap elements to even realize it is suppose to be a boolean field.
As far as I could see, the wrap parameter does not extend beyond the boolean type

[artemisart]

But no_wrap parameter is not a bool, it's an Optional[bool]. You will see it used everywhere with default_value(no_wrap, self.no_wrap, False) meaning it will use no_wrap, except if it's None then it will default to self.no_wrap, except if it's None again it will default to False.

[maestro-1]

Okay, that's fair enough @artemisart
Updated as requested

[EntilZha]

Thanks for the update @maestro-1, I agree with @artemisart that the behavior is slightly different. In general, I really like PRs that have single purpose changes, in this case only adding incremental typing information (versus also semantically changing code). The typing info at least to me looks obviously correct, so this LGTM to merge if you're done with changes. I'd certainly welcome a followup PR to add some type checking to the build pipeline (e.g., something like mypy, but suppressing errors for now, just to track progress towards fixing all type issues).

[maestro-1]

@EntilZha sounds good, glad to support. There's really no way to add all the typing at once, because the code base is pretty big and generally it takes a while to wrap your mind around everything that is going on.
I'll try to keep follow up PRs down to just type hints if you're opposed to changing the code.

The optional boolean boolean just really felt out of place, since for the most part it would accomplish the same thing as if you had used False straight away. For now though, I'll try to look more into why you chose optional boolean, instead of just boolean in case I might be missing something.
@EntilZha and @artemisart If you have clarifications for the behaviour being the way it is on this front, that would be awesome. I don't mind looking into and fixing up this behavior if that's the concern

[maestro-1]

hey @artemisart something wrong with the pipeline?
it's still waiting for status to be reported so I cannot merge

[artemisart]

@EntilZha has to allow actions to runs

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (95da04b) 98.10% compared to head (cf9d877) 98.10%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #190   +/-   ##
=======================================
  Coverage   98.10%   98.10%           
=======================================
  Files          12       12           
  Lines        2377     2380    +3     
=======================================
+ Hits         2332     2335    +3     
  Misses         45       45           

Files Changed	Coverage Δ
functional/pipeline.py	98.12% <100.00%> (+<0.01%)	⬆️
functional/transformations.py	100.00% <100.00%> (ø)
functional/util.py	98.43% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[EntilZha]

Ya, its annoying, I understand why it doesn't auto-run for first time contributors, but you'd think if I allow it once it would allow all future builds too...

FWIW, the difference on the None/False/True, is indicating whether its explicitly set to False/True rather than being unset (None), at least that is my recollection, its been a while since I've looked at that code.