Skip to content

Update: omit#84

Merged
Harris-Miller merged 5 commits into
developfrom
omit
Feb 4, 2024
Merged

Update: omit#84
Harris-Miller merged 5 commits into
developfrom
omit

Conversation

@Harris-Miller

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

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

@lautarodragan lautarodragan left a comment

Copy link
Copy Markdown

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

Harris-Miller commented Jan 20, 2024

Copy link
Copy Markdown
Collaborator Author

@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
Copy Markdown
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

@Harris-Miller Harris-Miller force-pushed the omit branch 2 times, most recently from 26e31cd to 1b64889 Compare January 21, 2024 00:13
@lautarodragan

Copy link
Copy Markdown

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
Copy Markdown
Collaborator Author

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

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