search

JamesHenry / typescript-estree

:sparkles: A parser that converts TypeScript source code into an ESTree-compatible form
https://jameshenry.blog
Other
84 stars 13 forks source link

feat: update TSMethodSignature node #104

Closed armano2 closed 5 years ago

armano2 commented 5 years ago

This PR changes TSMethodSignature node,

i'm just not sure if we should support: accessibility, export and static, this is syntax error TS1070:

Error:(2, 3) TS1070: 'public' modifier cannot appear on a type member.
Error:(2, 3) TS1070: 'export' modifier cannot appear on a type member.
armano2 commented 5 years ago

There was PR by @j-f1 with accessibility, export and static reported as errors #51, and i think we should not add unsupported properties to ast. this is just going to make ast unreadable.

we already have diagnostics in project.

JamesHenry commented 5 years ago

Can we not just enable TS1070 as part of this PR?

armano2 commented 5 years ago

we can but i was not sure if you want to xd

JamesHenry commented 5 years ago

Cool, yeah I think it makes sense based on all the precedent we have - it is not breaking by default, because you still have to opt into the diagnostics, even after we included them

armano2 commented 5 years ago

ok

armano2 commented 5 years ago

we are keeping fields for accessibility, export and static ?

JamesHenry commented 5 years ago

Hmm, I'm a bit confused now about the expected issue - nothing in the snapshot has changed after you have enabled the check?

armano2 commented 5 years ago

now you can see changes in ast

JamesHenry commented 5 years ago

I mean when we enable a new semantic check, we should be seeing a new diff in the semantic-errors snapshot. There isn't any change as a result of enabling the check here, which means we don't have coverage for it currently, right?

armano2 commented 5 years ago

its part of interface-property-modifiers but looks like there is way more errors there, and we are catching different one now

there are 1070, 1071, 1246, 2374, 1024, 2411

armano2 commented 5 years ago

i think we should should split this test to smaller one with separated errors :>

JamesHenry commented 5 years ago

Ok I see, that's the same as I had here:

/**
     * There are number of things that can be reported in this file, so it's not great
     * for comparison purposes.
     *
     * Nevertheless, Babel appears to throw on syntax that TypeScript doesn't report on directly.
     *
     * TODO: Investigate in more depth, potentially split up different parts of the interface
     */
'interface-with-all-property-types', // babel parse errors
JamesHenry commented 5 years ago

We could leave the diagnostics piece out of this PR for now then?

armano2 commented 5 years ago

kk, i'm going to split this to smaller separated errors in separated PR

armano2 commented 5 years ago

@JamesHenry i added PR with test cases for this https://github.com/JamesHenry/typescript-estree/pull/113

JamesHenry commented 5 years ago

Merged!

JamesHenry commented 5 years ago

:tada: This PR is included in version 15.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: