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

Better types for addJob #719

Merged
merged 2 commits into from
Mar 27, 2023
Merged

Conversation

nathanbabcock
Copy link
Contributor

Closes #718.

@nathanbabcock nathanbabcock changed the title Better types for addJob Better types for addJob Mar 8, 2023
@nathanbabcock
Copy link
Contributor Author

@Balearica I saw you active in a few of the other Typescript PRs 😉 Any thoughts on this one?

Are there any additional job type signatures I'm missing, aside from recognize, detect, and setParameters?

This change should be non-breaking (since it only affects types) and 100% backwards compatible with the existing type definitions. I've been using it manually in my projects for a few weeks now.

@Balearica
Copy link
Collaborator

@nathanbabcock Thank you for your contribution. I am not familiar with the nuances of Typescript/Intellisense but given your description this change seems helpful. I am suggesting one minor edit.

Are there any additional job type signatures I'm missing, aside from recognize, detect, and setParameters?

While technically scheduler.addJob can be used to call any method a worker contains, the only functions that should be used through scheduler.addJob are detect and recognize. This is because scheduler.addJob assumes all workers are homogenous and assigns jobs to an arbitrary worker. Using setParameters this way does not make sense because you would be changing the settings for a single worker (essentially) at random. Therefore, to avoid any confusion can you remove the line for setParameters? (I realize that ConfigResult was a return type before your edits, but this seems as good a time as ever to fix.)

@nathanbabcock
Copy link
Contributor Author

@Balearica thanks for the thoughtful reply. Your reasoning makes perfect sense.

I've removed setParameters from addJob, leaving detect and recognize as the two supported job types.

@Balearica Balearica merged commit b91cecb into naptha:master Mar 27, 2023
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.

More specific type definitions for addJob
3 participants