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

Add blob support to _assertDeepEqual #11

Merged
merged 8 commits into from
Feb 9, 2021

Conversation

mm-gmbd
Copy link
Contributor

@mm-gmbd mm-gmbd commented Aug 7, 2018

Add blob support for PR #10

mm-gmbd added 3 commits August 7, 2018 11:45
- Also, what it `type`?
- Switch back to the use of `type()` in the first `switch` to reduce impact on existing code
- `type(blob())` returns "meta", so added code to specifically catch "meta", and then perform `typeof`. If the result of `typeof` is "blob", then we handle the same as the case of "table"/"class"/"array", but otherwise we handle it as the default case
ppetrosh pushed a commit that referenced this pull request Nov 1, 2018
@alexey-nbl
Copy link

It is recommended to compare blobs by the one imp API method - crypto.equals() that would make the code much simpler.

Or do you need to print out the details of the comparison, like the "index" of the first non-equal byte, etc?

@mm-gmbd
Copy link
Contributor Author

mm-gmbd commented Feb 5, 2021

I don't think printing out the first non-equal byte is necessary, so yes this could be updated to use crypto.equals().

@mm-gmbd
Copy link
Contributor Author

mm-gmbd commented Feb 5, 2021

Updated to use crypto.equals() -- @alexey-nbl, does this look okay?

@alexey-nbl
Copy link

If you manually tested it, it's ready.

@mm-gmbd
Copy link
Contributor Author

mm-gmbd commented Feb 5, 2021

:) will let you know soon

@alexey-nbl
Copy link

oh... seems, it would be also good to add a check that value2 is a blob as well.
W/o such a check, value2.len() may return "ERROR: the index 'len' does not exist", if, for example, value2 is an integer.

@mm-gmbd
Copy link
Contributor Author

mm-gmbd commented Feb 5, 2021

I thought that was already in there, good catch!

@zandr zandr merged commit f4baca3 into electricimp:develop Feb 9, 2021
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