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

Forbid extra option and bubbling up the inner-most exception #183

Closed

Conversation

mishamsk
Copy link
Contributor

Hi,

This does two things:

Caveats:

  • The codecs code is pretty dense, I've tried to understand it without the comments, but do not feel confident enough to apply any changes there => the new features may or may not work for codecs
  • I dropped the ball on implementing the full path for sequences. I do not think that their going to be any noticeable performance penalty to track current sequence index, but to implement it, the whole unpack_collection must change from an expression generator to a full sub-builder. That's a bit too much for my time constraints;-)

I'd love this to be merged though. I think it adds quite a lot of value already.

Let me know what you think!

@pep8speaks
Copy link

pep8speaks commented Dec 17, 2023

Hello @mishamsk! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-12-17 16:04:54 UTC

@Fatal1ty
Copy link
Owner

Hi @mishamsk

Reviewing pull requests with mixed changes can be challenging. As a result, it has taken me some time to respond. Would you kindly consider splitting the pull request into separate parts? I appreciate your work and I'd like to review forbid_extra_keys part first since it looks less destructive. However, I would like to take more time to reflect on the other changes before making a decision. Thank you for your understanding.

@mishamsk
Copy link
Contributor Author

mishamsk commented Feb 3, 2024

Yeah, makes sense. Sorry for mixing these. Will get to it eventually, a bit busy now

@mishamsk
Copy link
Contributor Author

split into #198 & #199 , closing

@mishamsk mishamsk closed this Mar 13, 2024
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