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

refuse any device with web API version stricly less than 4 #205

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

vladak
Copy link
Contributor

@vladak vladak commented Mar 7, 2024

This change adds check for web API version on the device to make sure the web workflow backend can actually work.

@dhalbert dhalbert requested a review from tannewt March 7, 2024 22:08
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you are checking the version number. It shouldn't be hard to support version 2 though as well. Why not add support for version 2?

@vladak
Copy link
Contributor Author

vladak commented Mar 9, 2024

I like that you are checking the version number. It shouldn't be hard to support version 2 though as well. Why not add support for version 2?

I was thinking about it and decided for "not-this-PR", mainly because of limited capacity to write and test the web API ver. 2 code. Also, such change would likely require some refactoring and maybe adding another layer to the backends. Lastly, there is a question of how many versions can be reasonably supported without too much code bloat.

Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - yes, let's fix the immediate problem and there can be another PR for v2 support.

@dhalbert dhalbert merged commit 27ae1f9 into adafruit:main Mar 9, 2024
1 check 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.

3 participants