search

weskoop / Launchpad-Pro-Mk3-for-Bitwig

Launchpad Pro Mk3 Controller Script for Bitwig (Tested on 4+, should work on 3.2.5+)
GNU General Public License v3.0
48 stars 8 forks source link

Remove unnecessary "any" type and add missing typing info #4

Closed llgcode closed 2 years ago

llgcode commented 2 years ago

Just some quick review on typing, this adds some more completion.

Just a question on the "global" object ext, why is not included in launchpad itself?
Sorry I'm new to midi programming, there's 2 init functions the global init and launchpad init that access ext to initialize things that are part of launchpad, it seems to be the same thing.

thanks

llgcode commented 2 years ago

Precisely, these members below could be part of launchpad class (not ext) and initialized in launchpad:

...
grid: Grid;
buttons: Buttons;
drumPads: DrumPads;
...

and other Bitwig api members initialized outside of the launchpad class. I may be wrong

weskoop commented 2 years ago

At this point with a working project, I only makes sense to update syntax or refactor when there is some benefit gained.

There is a conscious separation of the ext.launchPad being the class connecting Bitwig Callbacks, too the hardware (layers, buttons, grid, drums, etc).

As it stands, this seems fine, but afaik, untested; you mentioned you haven't tried the script yet.

llgcode commented 2 years ago

Hi @weskoop, I've tested the script and it works fine. I agree that you shouldn't change a code that works. I just want to better understand the code to help with fixes. I don't want to offend you on the way you write your code, that's not my point. I can't resist suggesting changes to clarify my understanding of what you've done. You do with @Fannon a really good job :-). I think the benefit or refactoring, if you accept contributors, is to help better understanding what you've done.

Please reject the PR, if you think it's not relevant, no problem.

Fannon commented 2 years ago

From my point if view the current changes of this PR by @llgcode are useful because they do not really change the behavior of the code. It just makes the code more type safe and through that also a bit better documented / easier to work with.

weskoop commented 2 years ago

@llgcode Thanks for the PR, and confirming that you tested it. My comments were more in relation to to restructuring the ext global, this one looks good.