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 Key from always parsing (do not rename ("Key" is the name of a struct, which is upcased)) #1345

Closed
wants to merge 5 commits into from

Conversation

mariari
Copy link
Member

@mariari mariari commented May 10, 2023

Previously the key would always parse, even if the string is empty, this causes an issue where if we wanted an Optional Key, then it would always be parsed as Some, causing issues for various parts of the system.

Previously the key would always parse, even if the string is empty,
this causes an issue where if we wanted an Optional Key, then it would
always be parsed as Some, causing issues for various parts of the
system.
@mariari mariari requested a review from juped May 10, 2023 09:20
@juped
Copy link
Member

juped commented May 11, 2023

This change is correct per our discussion (the consumer expects Err in this case) but I want to track down why it fails two unit tests

@adrianbrink adrianbrink changed the title Fix Key from always parsing Fix key from always parsing May 12, 2023
@juped juped changed the title Fix key from always parsing Fix Key from always parsing (do not rename ("Key" is the name of a struct, which is upcased)) May 14, 2023
@juped
Copy link
Member

juped commented May 17, 2023

Looks like a few unit tests construct keys with empty segments; not sure yet whether it's a bug in the tests or the code

@cwgoes cwgoes mentioned this pull request May 18, 2023
10 tasks
@Fraccaman
Copy link
Member

what should we do here?

@mariari
Copy link
Member Author

mariari commented Jun 9, 2023

@juped is the new commit a fine solution to this matter?

@batconjurer
Copy link
Member

Looks like a few unit tests construct keys with empty segments; not sure yet whether it's a bug in the tests or the code

The empty key is to ensure we always match when iterating over keys.

@Fraccaman
Copy link
Member

closing this and merging via #1573

@Fraccaman Fraccaman closed this Jun 15, 2023
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.

4 participants