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 OmitStrict type for when you don't want to be permissive. #31

Merged
merged 2 commits into from
Jan 7, 2019
Merged

Add OmitStrict type for when you don't want to be permissive. #31

merged 2 commits into from
Jan 7, 2019

Conversation

seansfkelley
Copy link
Contributor

@seansfkelley seansfkelley commented Sep 19, 2018

I preferred the old behavior of Omit before it was made permissive, but I understand the use-cases for it. This PR re-introduces the original behavior under a different name.

The benefit that a stricter type has is primarily:

  • Preventing typos.
  • Allowing the compiler to pick up on rename refactors automatically.

Currently, permissive Omit acts as a "barrier" that prevents rename refactors from passing through, which means that any such refactor generates whole bunches of errors that have to be manually fixed. If the field in question is optional, this can actually introduce bugs.

I'm open to bike-shedding the name, but I'd like to have both types available so I don't have to pin to an old version of the library or re-declare my own strict variants!

@seansfkelley
Copy link
Contributor Author

The build is failing for reasons unrelated to my changes. I see these failures locally as well, but I'm not sure how to fix them.

@seansfkelley
Copy link
Contributor Author

Ping @pelotom, thoughts?

@seansfkelley
Copy link
Contributor Author

🏓 @pelotom

@pelotom
Copy link
Owner

pelotom commented Oct 18, 2018

Seems reasonable; if you can make CI happy I'll merge.

@seansfkelley
Copy link
Contributor Author

Perfect, thanks. However, the CI failures are caused by unrelated code from #24, perhaps something changed in 3.x that breaks it? Ping @fbartho, do you know what's up with Param* in 3.x?

@fbartho
Copy link
Contributor

fbartho commented Oct 18, 2018

@seansfkelley I'm didn't end up using ParamTypes in my codebase (yet) due to a lag in getting dependencies updated. So I'm just now upgrading to TS 3.1!

Separately, there exists this ticket:

I'll have time to look at ParamTypes in like a week or two, but if somebody could take a look at that issue & pr, and help us figure out the "right" fix, I certainly would be appreciative.

@Merott
Copy link

Merott commented Nov 26, 2018

What's the status on this? I find the behaviour of Omit undesirable, particularly because of reduced safety against typos and name refactors.

@seansfkelley
Copy link
Contributor Author

Master is currently broken due to the code introduced in #24. I do not understand how it works or how to fix it, so I have not done so. @pelotom has not given me the okay to merge it anyway even though the code in this PR is passing tests.

@seansfkelley
Copy link
Contributor Author

@pelotom looks like master fixed itself (new patch version of the language automatically pulled in?) so this PR now has passing builds. Could you take a look/merge/release it?

@pelotom pelotom merged commit 479541c into pelotom:master Jan 7, 2019
@seansfkelley
Copy link
Contributor Author

Thank you!

@ekilah
Copy link
Contributor

ekilah commented Mar 5, 2019

Just ran into this, thanks for bringing back the old functionality @seansfkelley .

@pelotom, why was the original behavior removed here? 761eff9 Can't tell from the commit message.

@pelotom
Copy link
Owner

pelotom commented Mar 5, 2019

@ekilah it’s just a different use case, where you don’t want to have to check that the properties you’re omitting are actually defined on the source type (I’ve found this to be the most common use case in practice but YMMV). In retrospect though I shouldn’t have redefined Omit but instead introduced a new operator for the relaxed version.

@ekilah
Copy link
Contributor

ekilah commented Mar 5, 2019

@pelotom 👍 agreed, but too late now 😛

I will say that after doing a find/replace in my project, there was only one place where I actually wanted Omit and not OmitStrict, and I'm actually skeptical of that line of code now anyways. Getting the old behavior back even caught a couple refactoring mistakes. So, YMMV indeed!

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.

5 participants