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

Do not use Omit type #19

Closed
wooorm opened this issue Feb 29, 2020 · 9 comments
Closed

Do not use Omit type #19

wooorm opened this issue Feb 29, 2020 · 9 comments
Labels
☂️ area/types This affects typings 🙅 no/wontfix This is not (enough of) an issue for this project 🐛 type/bug This is a problem

Comments

@wooorm
Copy link
Member

wooorm commented Feb 29, 2020

  types/index.d.ts:25:7
  ✖  25:7  Don't use Omit as a type. Prefer the Except type in the type-fest package instead as it's stricter.  @typescript-eslint/ban-types

dtslint 3 complains about these lines, what should we do?

entities: Partial<
Omit<StringifyEntitiesOptions, 'escapeOnly' | 'attribute' | 'subset'>
>

/cc @Rokt33r as you wrote the types

@wooorm wooorm added 🐛 type/bug This is a problem ☂️ area/types This affects typings 🔍 status/open labels Feb 29, 2020
@Rokt33r
Copy link
Contributor

Rokt33r commented Mar 3, 2020

In my opinion, the rule can be ignored. Omit doesn't check the omitted props exist in StringifyEntitiesOptions. So if we replace it with Except from type-fest, it would be a little bit helpful for us to detect trivial typos when we rename the omitted props in StringifyEntitiesOptions. And I don't think we would like to rename them that often. So i think we can just disable banning usage of Omit rather than introducing more dev deps.

@ChristianMurphy How do you think?

@ChristianMurphy
Copy link
Member

I'd agree, I think adding another devDependency would like cause more frustration than the benefit Except brings. We can probably safely disable this rule.

@Rokt33r
Copy link
Contributor

Rokt33r commented Mar 3, 2020

Awesome! I'll make a pull request to fix this issue.

@Rokt33r
Copy link
Contributor

Rokt33r commented Mar 3, 2020

@wooorm How can I reproduce the error message? Apparently we aren't using typescript-eslint rules for this repository.

@wooorm
Copy link
Member Author

wooorm commented Mar 3, 2020

I think updating dtslint to 3 gave me that error.
Xo does have typescript-eslint internally I believe

@wooorm
Copy link
Member Author

wooorm commented Mar 4, 2020

Ah, I turned them off while updating: 1b34d25

@Rokt33r
Copy link
Contributor

Rokt33r commented Mar 4, 2020

I see. I'll discard the comment and try to fix it again.

@Rokt33r
Copy link
Contributor

Rokt33r commented Mar 4, 2020

@wooorm Hmm I think we can just close this issue now. I've checked the code again and I like the fix currently applied. We use Omit only once in this package so I don't think it is worth to configure the rule in package.json.

@wooorm
Copy link
Member Author

wooorm commented Mar 4, 2020

OK! Thanks for looking into it & working on it!

@wooorm wooorm closed this as completed Mar 4, 2020
@wooorm wooorm added 🙅 no/wontfix This is not (enough of) an issue for this project and removed 🔍 status/open labels Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 🙅 no/wontfix This is not (enough of) an issue for this project 🐛 type/bug This is a problem
Development

No branches or pull requests

3 participants