Skip to content
This repository was archived by the owner on Jan 14, 2019. It is now read-only.

feat: update TSMethodSignature node #104

Merged
merged 9 commits into from
Jan 10, 2019

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Jan 6, 2019

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
Copy link
Contributor Author

armano2 commented Jan 8, 2019

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.

BREAKING CHANGE: This changes the AST for TSMethodSignature node
@JamesHenry
Copy link
Owner

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

@armano2
Copy link
Contributor Author

armano2 commented Jan 10, 2019

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

@JamesHenry
Copy link
Owner

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
Copy link
Contributor Author

armano2 commented Jan 10, 2019

ok

@armano2
Copy link
Contributor Author

armano2 commented Jan 10, 2019

we are keeping fields for accessibility, export and static ?

@JamesHenry
Copy link
Owner

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

@armano2
Copy link
Contributor Author

armano2 commented Jan 10, 2019

now you can see changes in ast

@JamesHenry
Copy link
Owner

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
Copy link
Contributor Author

armano2 commented Jan 10, 2019

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
Copy link
Contributor Author

armano2 commented Jan 10, 2019

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

@JamesHenry
Copy link
Owner

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
Copy link
Owner

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

@armano2
Copy link
Contributor Author

armano2 commented Jan 10, 2019

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

@armano2
Copy link
Contributor Author

armano2 commented Jan 10, 2019

@JamesHenry i added PR with test cases for this #113

@JamesHenry
Copy link
Owner

Merged!

@JamesHenry JamesHenry merged commit 337f405 into JamesHenry:master Jan 10, 2019
@armano2 armano2 deleted the method-signature branch January 10, 2019 23:51
@JamesHenry
Copy link
Owner

🎉 This PR is included in version 15.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants