Skip to content

feat: externalize ngWalker to support custom lint rules#658

Merged
mgechev merged 2 commits intomgechev:masterfrom
piyukore06:pr/externalizing-ngwalker
Jun 8, 2018
Merged

feat: externalize ngWalker to support custom lint rules#658
mgechev merged 2 commits intomgechev:masterfrom
piyukore06:pr/externalizing-ngwalker

Conversation

@piyukore06
Copy link
Contributor

@piyukore06 piyukore06 commented Jun 2, 2018

relates to #657

@mgechev
Copy link
Owner

mgechev commented Jun 2, 2018

Thanks for the quick PR! My idea was to externalize the content of the angular directory into a separate package. Does this cover your use case completely? Keep in mind that there are more visitor abstractions for traversal of template, style, and expression ASTs.

@piyukore06
Copy link
Contributor Author

piyukore06 commented Jun 3, 2018 •
edited
Loading

Hi @mgechev, Thank you for your input. I only needed NgWalker for my use case, but I think it makes sense to export the angular directory contents, there's so much goodness, everyone could use to build rules.
Just one question, When you are saying a separate package, do you mean npm module like @codelyzer/angular and @codelyzer/rules something like that?
or like a secondary entry point codelyzer/angular

@piyukore06 piyukore06 force-pushed the pr/externalizing-ngwalker branch from ae211a8 to 1b30481 Compare June 6, 2018 20:42
@mgechev
Copy link
Owner

mgechev commented Jun 6, 2018

@piyukore06 would you check why the build is failing?

@piyukore06
Copy link
Contributor Author

piyukore06 commented Jun 7, 2018

Yea I am trying but couldn't find anything.. locally the command runs without any problems.

mgechev
mgechev reviewed Jun 7, 2018
View reviewed changes
src/angular/index.ts Outdated
@@ -0,0 +1,21 @@
export * from './templates/basicTemplateAstVisitor';
export * from './templates/jitReflector';
Copy link
Owner

@mgechev mgechev Jun 7, 2018

Choose a reason for hiding this comment

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

I don't think we should export the jitReflector as part of the public API.

mgechev
mgechev reviewed Jun 7, 2018
View reviewed changes
src/angular/index.ts Outdated
export * from './templates/basicTemplateAstVisitor';
export * from './templates/jitReflector';
export * from './templates/recursiveAngularExpressionVisitor';
export * from './templates/referenceCollectorVisitor';
Copy link
Owner

@mgechev mgechev Jun 7, 2018

Choose a reason for hiding this comment

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

Better not export referenceCollectorVisitor.

mgechev
mgechev reviewed Jun 7, 2018
View reviewed changes
src/angular/index.ts
export * from './templates/referenceCollectorVisitor';
export * from './templates/templateParser';

export * from './styles/basicCssAstVisitor';
Copy link
Owner

@mgechev mgechev Jun 7, 2018

Choose a reason for hiding this comment

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

Can we keep only the basicCssAstVisitor exported?

mgechev
mgechev reviewed Jun 7, 2018
View reviewed changes
src/angular/index.ts Outdated
export * from './config';
export * from './expressionTypes';
export * from './metadata';
export * from './metadataReader';
Copy link
Owner

@mgechev mgechev Jun 7, 2018

Choose a reason for hiding this comment

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

Can we skip the metadataReader?

mgechev
mgechev reviewed Jun 7, 2018
View reviewed changes
src/angular/index.ts Outdated
export * from './metadataReader';
export * from './ngWalker';
export * from './ngWalkerFactoryUtils';
export * from './sourceMappingVisitor'; No newline at end of file
Copy link
Owner

@mgechev mgechev Jun 7, 2018

Choose a reason for hiding this comment

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

My guess is that the error comes from the missing newline.

@mgechev
Copy link
Owner

mgechev commented Jun 7, 2018

@piyukore06 if TypeScript won't complain, I'd suggest exporting only the base visitors without any extra internal APIs. This way, we'd be able to make changes in them without introducing issues for consumers of codelyzer.

@piyukore06 piyukore06 force-pushed the pr/externalizing-ngwalker branch from a12f3a7 to e5b16a8 Compare June 7, 2018 20:02
@piyukore06
Copy link
Contributor Author

piyukore06 commented Jun 8, 2018

@mgechev thank you for your comments! They lead me to a better understanding of, where to look and what should be exported.

@mgechev mgechev merged commit b79ea58 into mgechev:master Jun 8, 2018
@mgechev mgechev added this to the 4.4.0 - Ken Thompson milestone Jun 8, 2018
mgechev added a commit that referenced this pull request Jun 9, 2018
@mgechev
Merge branch 'master' of github.com:mgechev/codelyzer
615d57b 
* 'master' of github.com:mgechev/codelyzer:
  feat: externalizing template, css visitor abstractions and ngwalker (#658)
  feat(rule): add option in max-inline-declarations to limit animations lines (#569)
@piyukore06 piyukore06 deleted the pr/externalizing-ngwalker branch June 10, 2018 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgechev mgechev mgechev approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

4.4.0 - Ken Thompson

Development

Successfully merging this pull request may close these issues.

2 participants

@piyukore06 @mgechev