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

Improve coverage #2 #209

Merged
merged 6 commits into from
Nov 4, 2021
Merged

Improve coverage #2 #209

merged 6 commits into from
Nov 4, 2021

Conversation

dvermd
Copy link
Contributor

@dvermd dvermd commented Oct 29, 2021

This PR is based on the PR #207

  • Add the ability to deserialize HashMaps from Tape.
  • Add some more tests as part of Improve testing #57
  • Do not run coverage on test code

@dvermd
Copy link
Contributor Author

dvermd commented Oct 29, 2021

It seems that the code coverage is tagged as decreased because it uses the number of lines as metrics. With the --ignore-tests option of tarpaulin, there are less lines that are to be covered. Looking at Coveralls report, we can that the relevant column decreased just like the covered one.

If you want to review this change in its own PR to better see the coverage improvements of the other commits, let me know and I'll split the PR.

@dvermd dvermd mentioned this pull request Nov 1, 2021
@Licenser
Copy link
Member

Licenser commented Nov 3, 2021

Sorry for the late reply :) this looks great I'll update the branch and see if that solves the clippy complaining

@Licenser
Copy link
Member

Licenser commented Nov 3, 2021

Seems update was not enough :) can you address the clippy issues and then we can merge this :D

@dvermd
Copy link
Contributor Author

dvermd commented Nov 4, 2021

I had a hard time finding the clippy warnings the previous commits added. So I fixed most of the clippy warnings (most of them by clippy --fix).
The remaining warnings are from json! and json_typed! macro that swallow the error on inserting a (key, value) pair to a Value. I don't know how you want to handle this one but I think hiding the error this way will be misleading at some point for a user.

This one is also not addressed by this commit

warning: casting `i64` to `u64` may lose the sign of the value
  --> src/sse42/stage1.rs:76:17
   |
76 |         let b = -1_i64 as u64;
   |                 ^^^^^^^^^^^^^
   |

@dvermd dvermd force-pushed the improve_coverage2 branch 2 times, most recently from a253039 to b73ad3d Compare November 4, 2021 07:27
@Licenser
Copy link
Member

Licenser commented Nov 4, 2021

👍 awesome, I think leaving that out for now is fine. the let _ = ...insert in the macros isn't swallowing errors but rather swallowing the return value as we already know that the object is an object, and the content is a value since it's created by a macro :)

@Licenser Licenser merged commit ae48dc4 into simd-lite:main Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants