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

Drop special-case support for missing keys #7

Merged
merged 2 commits into from
Dec 20, 2021
Merged

Drop special-case support for missing keys #7

merged 2 commits into from
Dec 20, 2021

Conversation

zoul
Copy link
Contributor

@zoul zoul commented Dec 20, 2021

See #4 for longer discussion and rationale.

I’ve also added a commit that reformats the sources with Prettier – is my Prettier configuration wrong, or were the sources not completely formatted before? (We can drop this part if there’s something off with my Prettier.)

@tskj
Copy link
Owner

tskj commented Dec 20, 2021

The thing with prettier is that they change the formatting with every version. I usually just accept the churn, but I'm open to locking it down in a config file in the project if you know how.

Copy link
Owner

@tskj tskj left a comment

Choose a reason for hiding this comment

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

Thanks for putting this up!

I wonder if it's possible to give a better error message in the case that the key is actually missing and the decoder cannot handle undefined. As it stands now you get something like "undefined is not of type string, but of type undefined", which is kind of a weird error message. But I'm also not so sure if we can detect reliably (or explain in a good way to the user) that their decoder does not handle undefined and the key is missing (because it can throw for other reasons right?).

If not I will just merge this and take note of it as a potential improvement for later.

@zoul
Copy link
Contributor Author

zoul commented Dec 20, 2021

Prettier – if the formatting changes don’t annoy you, I’m fine with them. (I just wanted to make sure that this wasn’t some kind of formatting war between two incompatible configs :)

As for the error message, I don’t have a better solution now, I’m in favour of merging this and opening a new ticket to improve the error message later. Thank you!

@tskj
Copy link
Owner

tskj commented Dec 20, 2021

Alright, I've made a mental note to improve it, so merge it is!

I tried formatting it locally on my machine, and it didn't seem to change, so I'm fairly sure this is just prettier having changed its mind.

@tskj tskj merged commit af5d31b into tskj:master Dec 20, 2021
@zoul zoul deleted the feature/missing-keys branch December 20, 2021 20:31
@zoul
Copy link
Contributor Author

zoul commented Dec 20, 2021

(How soon will this make it to npm? I’ve got some work-in-progress code waiting for it, so I’d love to try it out :) But no pressure!)

@tskj
Copy link
Owner

tskj commented Dec 20, 2021

Should be any minute. Without tests, deploying is very quick ;)

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.

2 participants