Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: externalize ngWalker to support custom lint rules #658

Merged
merged 2 commits into from
Jun 8, 2018
Merged

feat: externalize ngWalker to support custom lint rules #658

merged 2 commits into from
Jun 8, 2018

Conversation

piyukore06
Copy link
Contributor

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

@mgechev
Copy link
Owner

mgechev commented Jun 6, 2018

@piyukore06 would you check why the build is failing?

@piyukore06
Copy link
Contributor Author

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

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

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 Outdated
export * from './templates/referenceCollectorVisitor';
export * from './templates/templateParser';

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

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

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

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

@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