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

Fix decode_ignored_any #4

Merged
merged 21 commits into from Apr 2, 2024
Merged

Conversation

DrewMcArthur
Copy link
Contributor

this beefs up the implementation for ignoring values, mostly lifted from serde's implementation. added a bunch of tests, which pass and these changes also fix the broken tests that led me to these changes in another repository of mine that's making use of this library.

  • would appreciate a closer look / double checking my implementations of some of the more base level helper functions here (e.g. peek_or_null, eat_char, etc.)
  • definitely take a look at the tests to see if there's anything i missed there that you'd like to cover
  • i think there's still some cleanup i can do; i'll take another pass at this later today.

@DrewMcArthur
Copy link
Contributor Author

closes #2

src/de.rs Outdated Show resolved Hide resolved
src/de.rs Show resolved Hide resolved
src/de.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/de.rs Outdated Show resolved Hide resolved
src/de.rs Outdated Show resolved Hide resolved
src/de.rs Outdated Show resolved Hide resolved
src/de.rs Outdated Show resolved Hide resolved
Copy link
Owner

@haydnv haydnv left a comment

Choose a reason for hiding this comment

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

Thanks Drew! Do you think you could bring some of the parsing improvements into the parse_* methods? For example parse_number could call parse_decimal, parse_exponent, etc. This could be in a separate PR if you prefer.

src/de.rs Outdated Show resolved Hide resolved
src/de.rs Outdated Show resolved Hide resolved
src/de.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@DrewMcArthur
Copy link
Contributor Author

@haydnv thanks for the review -

  1. i opened refactor parse_* methods #5 for refactoring the parse methods. i'd be open to it, but first would like to implement some benchmark tests to make sure any changes only speed things up!
  2. I think I addressed all the comments. would appreciate another glance to make sure everything is covered.
  3. only things missing are some "expected failures" tests. i can add those here if you'd like, or we can open an issue to make sure they eventually get added.

src/de.rs Outdated Show resolved Hide resolved
@haydnv
Copy link
Owner

haydnv commented Mar 27, 2024

It would be great if you can add the expected failure tests in this PR

@DrewMcArthur
Copy link
Contributor Author

okay! @haydnv sorry for the delay. i've added a bunch more test cases, and ran cargo llvm-cov --features all --output-path lcov.info --lcov to verify that all the code paths in these ignore_... functions are covered.

@haydnv
Copy link
Owner

haydnv commented Apr 2, 2024

Thanks Drew, great work!

@haydnv haydnv merged commit 8fbf971 into haydnv:main Apr 2, 2024
10 checks 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.

None yet

2 participants