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

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

Merged
merged 1 commit into from Jan 22, 2022

Conversation

llgcode
Copy link
Contributor

@llgcode llgcode commented Jan 21, 2022

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
Copy link
Contributor Author

llgcode commented Jan 21, 2022

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
Copy link
Owner

weskoop commented Jan 22, 2022

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
Copy link
Contributor Author

llgcode commented Jan 22, 2022

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
Copy link
Contributor

Fannon commented Jan 22, 2022

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 weskoop linked an issue Jan 22, 2022 that may be closed by this pull request
@weskoop weskoop merged commit ba47af3 into weskoop:main Jan 22, 2022
@weskoop
Copy link
Owner

weskoop commented Jan 22, 2022

@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.

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.

TypeScript Typing Issues
3 participants