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

feat: add HTTP API GET for module variables #3119

Merged
merged 6 commits into from
Nov 7, 2024

Conversation

GuideOTA
Copy link
Contributor

@GuideOTA GuideOTA commented Oct 29, 2024

resolves #2830

added an API GET command to return value of variables from modules rather than just custom variables

This is my first experience with GIT/TypeScript and honestly I have no Idea if what I added works or how to go about testing. any guidance would be appreciated, I am enthusiastic but ignorant.

@CLAassistant
Copy link

CLAassistant commented Oct 29, 2024

CLA assistant check
All committers have signed the CLA.

@GuideOTA GuideOTA marked this pull request as ready for review October 29, 2024 20:53
@GuideOTA GuideOTA changed the title Adds API GET for module variables Adds API GET for module variables resolves #2830 Oct 30, 2024
@GuideOTA
Copy link
Contributor Author

GuideOTA commented Nov 1, 2024

Tested locally and the GET returns the value of module variables correctly

@thedist
Copy link
Member

thedist commented Nov 2, 2024

For better User Experience (and any developers who have to work on your code later), I would suggest changing the naming you're using in the API and docs for module, as that could lead to potential confusion as you're not getting the variable values of a module your getting the variable values of a specific instance of a module based on its label.

It's very possible that a user could have multiple connections to the same module, each with different values for the same variable, so why not make things clearer that the API requires the connection label.

For consistency it would be best if you adhere to the naming scheme already used within Companion.

Changed all refrences to "module" to instead be "label" or "connection label" to make it more clear to users and better fit companion conventions
@Julusian
Copy link
Member

Julusian commented Nov 6, 2024

Looking good!
Could you try adding some unit tests in https://github.com/bitfocus/companion/blob/main/companion/test/Service/HttpApi.test.ts for this new endpoint? The tests for the custom-variable get will be a good reference of what cases to test and how it should behave

Added unit tests for the module variable API call using the custom-variable get tests as a guideline
@GuideOTA
Copy link
Contributor Author

GuideOTA commented Nov 6, 2024

Thanks for the guidance! working on this issue has been a big confidence booster for me. Hopefully the first of many contributions.

@Julusian Julusian changed the title Adds API GET for module variables resolves #2830 feat: add HTTP API GET for module variables Nov 7, 2024
@Julusian Julusian merged commit 98a0e32 into bitfocus:main Nov 7, 2024
1 of 2 checks passed
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.

API: GET-Requests for all variables
4 participants