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

feat(lint): add more linters #30

Closed
wants to merge 2 commits into from
Closed

feat(lint): add more linters #30

wants to merge 2 commits into from

Conversation

jimexist
Copy link
Contributor

What this PR does
add more linters

why we need it
cover more lint cases

Special notes for your reviewer:

@coveralls
Copy link

coveralls commented Nov 17, 2017

Coverage Status

Coverage remained the same at 75.776% when pulling 6c50387 on add-linter into 16ac175 on master.

@jimexist jimexist requested a review from a team November 17, 2017 09:45
@jimexist jimexist self-assigned this Nov 19, 2017
@coveralls
Copy link

coveralls commented Nov 19, 2017

Coverage Status

Coverage remained the same at 92.34% when pulling 6f8fcb6 on add-linter into 1497682 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.34% when pulling e435390 on add-linter into 1497682 on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.34% when pulling e435390 on add-linter into 1497682 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.34% when pulling e435390 on add-linter into 1497682 on master.

@ddysher
Copy link
Member

ddysher commented Nov 19, 2017

Please use our commit style

add more linters

Please remove such commits

Merge branch 'master' into add-linter

What's the criteria for adding a new linter?

@jimexist
Copy link
Contributor Author

jimexist commented Nov 19, 2017

@ddysher can we ignore the specific commit messages, if we enforce squash commits for pull requests? So that:

  • for individual comments before merging, we can revert or cherry-pick one specific pervious commit, if there is a request to change from PR comment
  • we can better identify bugs introduced by merging from master if they are causing passing builds to start to fail in a PR
  • upon merging, there will still be only one merge commit per PR, resulting a clean master branch (i.e. one commit per feature).

@jimexist
Copy link
Contributor Author

jimexist commented Nov 19, 2017

criteria for adding linters:

  • they make sense and are commonly supported
  • until they introduced too much overhead in future development (e.g. enforcing a specific comment tag on EVERY function and struct).

@coveralls
Copy link

coveralls commented Nov 19, 2017

Coverage Status

Coverage remained the same at 92.34% when pulling 186d391 on add-linter into 1497682 on master.

@coveralls
Copy link

coveralls commented Nov 19, 2017

Coverage Status

Coverage remained the same at 92.34% when pulling 186d391 on add-linter into 1497682 on master.

@supereagle
Copy link
Member

@jimexist Please fork this repo and create the PR from your forked repo.

@ddysher
Copy link
Member

ddysher commented Nov 20, 2017

Good catch! @supereagle

@jimexist Please fork this repo and create the PR from your forked repo.

@coveralls
Copy link

coveralls commented Nov 22, 2017

Coverage Status

Coverage remained the same at 92.34% when pulling 539d203 on add-linter into afcfe8a on master.

@ddysher
Copy link
Member

ddysher commented Nov 22, 2017

@jimexist Since there is already conflict, can we close the PR and then create a new PR from your own fork?

@jimexist jimexist closed this Nov 22, 2017
@jimexist jimexist deleted the add-linter branch November 22, 2017 13:16
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.

None yet

4 participants