-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): add option to no-object-literal-type-assertion rule #87
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
feat(eslint-plugin): add option to no-object-literal-type-assertion rule #87
Conversation
packages/eslint-plugin/docs/rules/no-object-literal-type-assertion.md
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #87 +/- ##
==========================================
+ Coverage 95.08% 95.09% +<.01%
==========================================
Files 49 49
Lines 2483 2487 +4
Branches 371 371
==========================================
+ Hits 2361 2365 +4
Misses 73 73
Partials 49 49
|
@armano2 Conflicts now just FYI |
i don't like name of option here: |
|
There may be another use case here that needs consideration: return values. Consider the following code: // Returns either Thing or Partial<Thing>
function getNewOrExistingThing(id?: number): Thing | Partial<Thing> {
// if an id was not passed, return a Partial<Thing> (new)
if (isNaN(id)) return { ... } as Partial<Thing>;
// otherwise, return a Thing (existing)
return { ... } as Thing;
} Unlike the Would the proposed |
Since TypeScript uses a structural type system, the returned object will pass the type check as long as it has all the required properties of |
@scottohara Following on from @j-f1 comment, I am not sure about your example. Let's take the example where there is no extra I would say that the second form is what you would do - allowing the type assertion is really not desirable in most cases, as it opens you up to potential future issues after refactorings, and so having a linting option to allow it doesn't seem to make sense to me. Please let me know if I am missing something. In the rare case where you do need the type assertion behaviour as a return value, using an Let me know what you think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM.
We can always add more cases later
@JamesHenry - thank you for your playground example, and as usual you are 100% spot on. The second form does indeed (at the cost of one extra |
I'll cut a full release after this goes in, we've merged in quite a few fixes since v1 now |
@JamesHenry its way easier now to test and fix issues (when we have control over all packages) |
Awesome, that was the hope :) I am very happy with how our monorepo is shaping up! I've released https://github.com/typescript-eslint/typescript-eslint/releases/tag/v1.1.0 |
This PR adds new option to
no-object-literal-type-assertion
to disable it when used in call or new expression.@IlyaSemenov
fixes: #66