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

Change typings from Array.<number> to ArrayBuffer in third_party/pcsc-lite/naclport/js_client/src/api.js #805

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

ViktoriiaKovalova
Copy link
Collaborator

No description provided.

@ViktoriiaKovalova ViktoriiaKovalova changed the title Change typings for atr parameter in API. Change typings for atr parameter in API.SCARD_READERSTATE_OUT Jun 23, 2023
@emaxx-google emaxx-google self-requested a review June 23, 2023 09:33
@emaxx-google
Copy link
Collaborator

Did only these three places have incorrect signatures, or effectively the whole file used !Array.<number> incorrectly? I don't have an answer off the top of my head, but it might be an unintentional consequence of #222...

@ViktoriiaKovalova
Copy link
Collaborator Author

I didn't check what is actually returned for other functions but looking at #222, I guess in other places !Array.<number> is also used incorrectly.

@ViktoriiaKovalova ViktoriiaKovalova changed the title Change typings for atr parameter in API.SCARD_READERSTATE_OUT Change typings from Array.<number> to ArrayBuffer in third_party/pcsc-lite/naclport/js_client/src/api.js Jun 23, 2023
Copy link
Collaborator

@emaxx-google emaxx-google left a comment

Choose a reason for hiding this comment

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

I've spent some time looking how we can write a test for this change, but seems like a nontrivial investment in the moment (as we'd need an integration-like C++-to-JS test, which we do have but only for a different component, and this only for NaCl which we cannot test automatically on bots).

But I've performed a manual test by adding test assertions, and the change seems to be correct. Hence LGTM, and thanks for catching and fixing this.

@emaxx-google emaxx-google merged commit 216d28c into GoogleChromeLabs:main Jun 26, 2023
8 of 9 checks passed
@ViktoriiaKovalova ViktoriiaKovalova deleted the fix-typings branch July 12, 2023 14:39
@emaxx-google emaxx-google added this to Done in Code Quality Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants