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

Update: omit #84

Merged
merged 5 commits into from
Feb 4, 2024
Merged

Update: omit #84

merged 5 commits into from
Feb 4, 2024

Conversation

Harris-Miller
Copy link
Collaborator

@Harris-Miller Harris-Miller commented Jan 3, 2024

typing omit same as pick

Additional fix for optional properties for pick

@Harris-Miller Harris-Miller mentioned this pull request Jan 3, 2024
@Harris-Miller
Copy link
Collaborator Author

@lautarodragan it was easier to fix your issue #81 (comment) by making my own MR

The problem as far as I can tell is Typescript itself. Record<ElementOf<Names>, any> won't take an object if one of the keys is optional, but making it Partial<Record<ElementOf<Names>, any>> will allow objects that are missing keys from the array

While not the ideal solution, what I was able to do is make it return never for the latter case by comparing ElementOf<Names> to keyof U in the return type

Copy link

@lautarodragan lautarodragan left a comment

Choose a reason for hiding this comment

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

Nice! I've tested this PR my code and it works as expected.

Maybe unrelated, but it didn't feel nice to see my PR being closed, replaced by another, after the effort and time I spent on it. I don't assume any bad intentions, but definitely doesn't feel welcoming.

Regardless, I appreciate that omit will now be type-safe, and that you found a way to implement so; and, more broadly, your work bringing type safety to Ramda.

@Harris-Miller
Copy link
Collaborator Author

Harris-Miller commented Jan 20, 2024

@lautarodragan

Maybe unrelated, but it didn't feel nice to see my PR being closed, replaced by another, after the effort and time I spent on it. I don't assume any bad intentions, but definitely doesn't feel welcoming.

Thank you for the constructive criticism, I'll work on my communication so for the future instead of close+replace we'll keep the initial MR for these cases

@Harris-Miller
Copy link
Collaborator Author

@lautarodragan

Nice! I've tested this PR my code and it works as expected.

I pushed some additional changes, can you retest and let me know if everything is still working as expected, please and thank you

@lautarodragan
Copy link

Thank you for the constructive criticism, I'll work on my communication so for the future instead of close+replace we'll keep the initial MR for these cases

Thanks ❤️

I pushed some additional changes, can you retest and let me know if everything is still working as expected, please and thank you

I just did a couple of manual tests in my code base and it works perfectly! It correctly reports errors when I try to access fields I've omitted, both with the curried and non-curried versions; and the non-omitted fields are still accessibly and correctly typed.

When including in the omit list a field that does not exist in the object type it infers the type as never and stops there, too.

I think this even uncovered an existing bug in the code base.

🚀

@Harris-Miller
Copy link
Collaborator Author

@lautarodragan accepting that as a Virtual "Approve" and merging

@Harris-Miller Harris-Miller merged commit f05db5a into develop Feb 4, 2024
3 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.

2 participants