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 test for log::Level deserialization #188

Merged
merged 4 commits into from
Nov 28, 2022

Conversation

matthiasbeyer
Copy link
Collaborator

This is an attempt to tackle #136

@TheNeikos
Copy link

I believe to make this complete case-insensitive you would need to fix the check here:

.find(|&&s| s == name)

As this is checked for before even the actual log crate gets to deserialize, thus making it case-sensitive even if the log crate might not be.

@matthiasbeyer
Copy link
Collaborator Author

Okay, so I have a patch, but it does not work. It is here

It is way harder to implement this, because the relevant code is deeply tied into serde and we cannot simply add settings to it. The problematic line is matthiasbeyer@33b8cf0#diff-be1c878c753064b7eaddf4a10ba4215a93dd1817d31e6788691cdb29752fd612R545 . I could, in theory, remove the From implementation and add some more powerful abstraction here, but the actual problem is not only this line, but also the Deserialize impl for Value, where we need to implement deserialization without having a Value object (or any other), so we cannot have settings at all.

I must say, I'm not sure whether this is possible at all, without rewriting large parts of this crate.

@TheNeikos
Copy link

TheNeikos commented Mar 28, 2021

My question is why not let the enum try an deserialize itself? Why is the crate checking for a specific variant at all? You could try constructing a StrDeserializer directly.

@matthiasbeyer
Copy link
Collaborator Author

TBH I don't know yet why this is done. I hope I get the chance to investigate this.

Signed-off-by: Matthias Beyer <[email protected]>
This test no longer works because we removed case sensitivity.

Signed-off-by: Matthias Beyer <[email protected]>
@matthiasbeyer matthiasbeyer changed the title Fix log::Level deserialization case sensitivity Add test for log::Level deserialization Nov 28, 2022
@matthiasbeyer
Copy link
Collaborator Author

So the original title of this PR does not apply anymore. The patches here only test whether log levels can be deserialized.

I'll change the title accordingly. Still nice to get these patches in, after 1.5 years 😆

@matthiasbeyer matthiasbeyer merged commit 834a163 into mehcode:master Nov 28, 2022
@matthiasbeyer matthiasbeyer deleted the log-deser branch November 28, 2022 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted PR for hacktoberfest help wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants