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

CPPLintBear.py: Add check_invalid_config test #1165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CPPLintBear.py: Add check_invalid_config test #1165

wants to merge 1 commit into from

Conversation

SarthakNijhawan
Copy link
Contributor

CPPlintbear supports only use of spaces with indent_size = 2, according
to the CPPLint style-guide ,previously no check was present.

Fixes #896

@gitmate-bot
Copy link
Collaborator

Thanks for your contribution!

Reviewing pull requests take really a lot of time and we're all volunteers. Please make sure you go through the following check list and complete them all before pinging someone for a review.

As you learn things over your Pull Request please help others on the chat and on PRs to get their stuff right as well!

@SarthakNijhawan
Copy link
Contributor Author

SarthakNijhawan commented Dec 18, 2016

@jayvdb I have created the pull request .. please take a look ...and suggest the necessary ... 😃
and @Mixih here I have created another .... pls have a look .. Thnks for the help though .. 😄

cpplint_include: typed_list(str)=()):
cpplint_include: typed_list(str)=(),
indent_size: int=2,
use_spaces: bool=2):
Copy link
Member

Choose a reason for hiding this comment

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

bool = True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry ...yea ....True , my bad ! 😛

@@ -2,6 +2,10 @@
from coalib.bears.requirements.PipRequirement import PipRequirement
from coalib.settings.Setting import typed_list

def check_invalid_config(use_spaces, indent_size):
if not use_spaces or indent_size != 2:
return False
Copy link
Member

Choose a reason for hiding this comment

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

wait... does this mean that indent_size cannot be changed ?, then why do we need it 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the issue demanded it #896 .... thats why I did the very way ..

Copy link
Member

Choose a reason for hiding this comment

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

@AsnelChristian We need to verify that the user has not changed it from indent_size=2. If they did, the bear will not honour their configuration , and the user should be informed.

"""
:param max_line_length: Maximum number of characters for a line.
:param cpplint_ignore: List of checkers to ignore.
:param cpplint_include: List of checkers to explicitly enable.
:param use_spaces: Bool value whether to use spaces or not.
Copy link
Member

Choose a reason for hiding this comment

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

True if spaces are to be used instead i think that is what is used almost everywhere
maybe you should use that for consistency...
and also align the param docs ( the should start at the same column)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok , i'll change that ...

"""
if not check_invalid_config(use_spaces, indent_size):
raise ValueError("CPPLint doesn\'t support indent_size != 2 or "
+ "not using spaces.")
Copy link
Member

Choose a reason for hiding this comment

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

really i don't see the point of having this setting if the user cannot customize it... what do you think @SarthakNijhawan ?

@Mixih
Copy link
Member

Mixih commented Dec 20, 2016

Disable your circle builds as they don't have the right number of containers...

Copy link
Member

@Mixih Mixih left a comment

Choose a reason for hiding this comment

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

Also builds are failing because the bear doesn't complain when bad indent options are passed.

@Mixih
Copy link
Member

Mixih commented Dec 20, 2016

Please disable your circle building

@SarthakNijhawan
Copy link
Contributor Author

@Mixih Done!

"""
:param max_line_length: Maximum number of characters for a line.
:param cpplint_ignore: List of checkers to ignore.
:param cpplint_include: List of checkers to explicitly enable.
:param use_spaces: True if spaces are to be used instead.
Copy link
Member

Choose a reason for hiding this comment

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

This will be used in documentation. It should say that the only acceptable value is True. Likewise for indent_size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll change it.

@sils
Copy link
Member

sils commented Jan 3, 2017

Adds is the wrong tense, Add

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

27 similar comments
@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants