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.

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

Feature/happy lint #1105

Closed
wants to merge 6 commits into from

Conversation

michaeljota
Copy link

@michaeljota michaeljota commented Oct 15, 2016

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

tslint update

  • What is the current behavior? (You can also link to an open issue here)
    • Tslint 3.9.x have a bug while dealing with wilcards
    • Codelyzer rules have not been used
    • Recommended rules hard coded.
  • What is the new behavior (if this is a feature change)?
    • TSlint update to @next
    • Recommended Codelyzer rules being used in a separate file.
    • Extend recommended rules.
    • Update all code to follow new rules
  • Other information:

This somehow is related to #1091 and #1084. But instead of just follow the style guide I updated the TSLint to linter the style guide with codelyzer.

@michaeljota
Copy link
Author

Ok. I did not see #185 before this. BUT, there is now an official Angular 2 Style Guide there is even a TSLint plugin to use them, that by the way this seed "use", named codelyzer, with the apropiated rules, everything should be fine.

I just wanted to help.

@katallaxie
Copy link
Contributor

I did merged #185 and we have to adjust some things.

@michaeljota
Copy link
Author

michaeljota commented Oct 15, 2016

I saw it. But as I say, this is not just "styling" the seed to Angular2 style guide, but instead, linting it, and fix the files to the linting, to Codelyzer, that happens to be the plugin to follow Angular2 style guide.

BTW #185 it's not a PR, but an open issue dating Dec-28. This seed says that use Codelyzer, I guess that this should be true, other than be included in the package.json

@michaeljota
Copy link
Author

Now that you merge #1091 I guess this have better chances, since now it only config the linter to use recommended rules, from TSLint and Codelyzer.

Copy link
Contributor

@katallaxie katallaxie left a comment

Choose a reason for hiding this comment

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

We should have all necessary linting rules in the tslint.json.

@michaeljota
Copy link
Author

michaeljota commented Oct 16, 2016

I can do that. I guess that it's better that it's clear where the rules come from, if there are coming from external plugin.

@katallaxie I'll try to explain myself with a use case: If someone want to add another set of linting rules from other source, says tslint-eslint-rules, those rules would be grouped in a external file, ex: eslint.tslint.json, in the config/tslint/ foler, and just extends those set of rules. With this everybody know where exactly the rules are coming from and where to ask if some rule it's being misused or something.

@katallaxie
Copy link
Contributor

@michaeljota I just thought the same, yet coming from a different angle. I was think that in this starter, a single point for the rules is the way to go. So, I would just keeping everything in tslint.json.

@michaeljota
Copy link
Author

michaeljota commented Oct 17, 2016

I see your point. I have a question then, should the recommended rules still explicitly be available or should they be extended?

@katallaxie I have not update this, as I want to know the above information. But I think, you already had update partially this.

@joshwiens
Copy link
Contributor

Other dependencies do not support tslint 4.x, resubmit a PR when both Codelyzer & tslint-loader all support the newer version of tslint. Merging something for 4.x / @next will fail peerDeps breaking nodejs 4.x builds.

@joshwiens joshwiens closed this Nov 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants