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

Fix: visiting superTypeParameters in classes #561

Merged
merged 4 commits into from
Nov 28, 2018

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Nov 21, 2018

This PR is adding superTypeParameters to class like types ClassDeclaration, ClassExpression, TSAbstractClassDeclaration and fixes order of TSAbstractClassDeclaration

@armano2 armano2 mentioned this pull request Nov 21, 2018
14 tasks
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@armano2
Copy link
Contributor Author

armano2 commented Nov 21, 2018

@platinumazure should i put each one of missing visitor key in each own PR or should i make one big PR?

@platinumazure
Copy link
Member

@armano2 Up to you. Please spare a thought for the poor reviewer that has to review a 1000+ line PR 😄 But if it's not that big, you could do one or a few PRs rather than one per node type.

@armano2
Copy link
Contributor Author

armano2 commented Nov 21, 2018

its more than 1k line with tests xd, i'm asking because some stuff do collide

lets say, TSAbstractClassProperty and TSAbstractClassDeclaration is missing decorators, but i already changed those lines here, and we are going to have conflicts,

i made overview in #555

@platinumazure
Copy link
Member

@armano2 I would say do what is most convenient for you and worst case, we can ask for changes. I'm not super worried-- I trust you will have good judgment. (If there are areas of code where collisions would happen, let's do those in combined PRs to make things easier.)

its already supported in visitClass
its using same logic as ClassDeclaration
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@platinumazure platinumazure merged commit cc92044 into eslint:master Nov 28, 2018
@platinumazure
Copy link
Member

@armano2 Merged, thanks for contributing!

@armano2 armano2 deleted the patch-4 branch November 28, 2018 21:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants