CheckstyleBear: Add invalid configuration check #917

tylfin · 2016-10-20T20:30:11Z

Closes #898 ensuring that if the checkstyle_config is google, then use_spaces has to be true and indent_size is 2.

gitmate-bot · 2016-10-20T20:30:27Z

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.

I have rebased properly. Please see this for a tutorial.
I have gone through the commit guidelines and I've followed them:
- I have followed the guidelines for the commit shortlog.
- I have followed the guidelines for the commit body.
- I have included the issue URL.
- I have used the Fixes keyword if the commit fixes a real bug and the Closes keyword if it adds a feature/enhancement. See more here.
I have run all the tests and they all pass. That includes that all CI (below) is green - if GitMate issues appear you will have to amend your commits, pushing new commits on top is not sufficient!

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

gitmate-bot · 2016-10-20T20:30:39Z

Comment on 6a010b9.

No newline between shortlog and body at HEAD.

GitCommitBear, severity NORMAL, section commit.

gitmate-bot · 2016-10-20T20:31:12Z

Comment on 383b35e.

No newline between shortlog and body at HEAD.

GitCommitBear, severity NORMAL, section commit.

aptrishu · 2016-10-23T13:38:01Z

Hey @tylfin , Please follow the writing good commits guideline. you can go over here for that http://coala.io/commit . Your commit structure is not right.

First make your commit structure right. Then someone will review you PR very soon. :)

sils · 2016-10-25T16:59:23Z

@tylfin can you add the Closes note to your message according coala.io/commit?

sils · 2016-10-25T17:01:42Z

bears/java/CheckstyleBear.py

@@ -10,6 +10,13 @@
    "geosoft": "http://geosoft.no/development/geosoft_checks.xml"}


+def invalid_configuration(checkstyle_configs, use_spaces, indent_size):
+    if checkstyle_configs != 'google':
+        return


rather return False right? Also you could just return checkstyle_configs == 'google' and ... to make it a oneliner, right?

also I think you should research for the other configs on what's invalid for those and include those cases in here

gitmate-bot · 2016-10-25T17:31:58Z

bears/java/CheckstyleBear.py

@@ -10,6 +10,10 @@
    "geosoft": "http://geosoft.no/development/geosoft_checks.xml"}


+def invalid_configuration(checkstyle_configs, use_spaces, indent_size):
+    return checkstyle_configs == 'google' and (not use_spaces or indent_size != 2)


Line is longer than allowed. (82 > 79)

LineLengthBear, severity NORMAL, section linelength.

Makman2 · 2016-10-25T18:18:32Z

bears/java/CheckstyleBear.py

@@ -10,6 +10,11 @@
    "geosoft": "http://geosoft.no/development/geosoft_checks.xml"}


+def invalid_configuration(checkstyle_configs, use_spaces, indent_size):
+    return checkstyle_configs == 'google' and \
+        (not use_spaces or indent_size != 2)


would you mind using braces instead of \? :)
-->

return (... continues here)

Makman2 · 2016-10-25T18:19:24Z

bears/java/CheckstyleBear.py

+        self, filename, file, config_file,
+        checkstyle_configs: known_checkstyle_or_path="google",
+        use_spaces: bool = True, indent_size: int = 2
+    ):


please no spaces around = for default arguments + align the ): onto the previous line :)

Makman2 · 2016-10-25T18:19:35Z

bears/java/CheckstyleBear.py

@@ -64,6 +71,9 @@ def create_arguments(
            - geosoft - The Java style followed by GeoSoft. More info at
              <http://geosoft.no/development/javastyle.html>
        """
+        if invalid_configuration(checkstyle_configs, use_spaces, indent_size):
+            raise ValueError('Invalid configuration! Cannot proceed.')


could you be a bit more precise with the message?

What would you recommend?

"Google checkstyle config conflicts with your use_spaces or indent_size"

It is not ideal. Maybe invalid_configuration should raise the exception, so that the message can be:

"Google checkstyle config does not support use_spaces|indent_size=x"

Makman2 · 2016-10-25T18:20:00Z

tests/java/CheckstyleBearTest.py

@@ -22,6 +22,9 @@ def setUp(self):
        self.empty_config = os.path.join(test_files,
                                         "checkstyle_empty_config.xml")

+    def tearDown(self):
+        self.section = Section("test section")


why do you create a new section on tearDown?

Makman2 · 2016-10-25T18:20:48Z

some minor things left to do, but the direction is right 👍 :D

gitmate-bot · 2016-10-25T18:49:25Z

bears/java/CheckstyleBear.py

-            self, filename, file, config_file,
-            checkstyle_configs: known_checkstyle_or_path="google"):
+    def create_arguments(self, filename, file, config_file,
+        checkstyle_configs: known_checkstyle_or_path="google",


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/bears/java/CheckstyleBear.py +++ b/bears/java/CheckstyleBear.py @@ -48,8 +48,8 @@ "checkstyle.jar") def create_arguments(self, filename, file, config_file, - checkstyle_configs: known_checkstyle_or_path="google", - use_spaces: bool=True, indent_size: int = 2): + checkstyle_configs: known_checkstyle_or_path="google", + use_spaces: bool=True, indent_size: int = 2): """ :param checkstyle_configs: A file containing configs to use in ``checkstyle``. It can also

gitmate-bot · 2016-10-25T18:49:26Z

bears/java/CheckstyleBear.py

@@ -10,6 +10,11 @@
    "geosoft": "http://geosoft.no/development/geosoft_checks.xml"}


+def invalid_configuration(checkstyle_configs, use_spaces, indent_size):
+    return (checkstyle_configs == 'google' and
+        (not use_spaces or indent_size != 2))


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/bears/java/CheckstyleBear.py +++ b/bears/java/CheckstyleBear.py @@ -12,7 +12,7 @@ def invalid_configuration(checkstyle_configs, use_spaces, indent_size): return (checkstyle_configs == 'google' and - (not use_spaces or indent_size != 2)) + (not use_spaces or indent_size != 2)) def known_checkstyle_or_path(setting):

gitmate-bot · 2016-10-25T18:51:01Z

bears/java/CheckstyleBear.py

@@ -43,8 +48,9 @@ def setup_dependencies(self):
            "checkstyle.jar")

    def create_arguments(
-            self, filename, file, config_file,
-            checkstyle_configs: known_checkstyle_or_path="google"):
+        self, filename, file, config_file,


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/bears/java/CheckstyleBear.py +++ b/bears/java/CheckstyleBear.py @@ -48,9 +48,9 @@ "checkstyle.jar") def create_arguments( - self, filename, file, config_file, - checkstyle_configs: known_checkstyle_or_path="google", - use_spaces: bool=True, indent_size: int = 2): + self, filename, file, config_file, + checkstyle_configs: known_checkstyle_or_path="google", + use_spaces: bool=True, indent_size: int = 2): """ :param checkstyle_configs: A file containing configs to use in ``checkstyle``. It can also

gitmate-bot · 2016-10-25T18:51:02Z

bears/java/CheckstyleBear.py

@@ -10,6 +10,11 @@
    "geosoft": "http://geosoft.no/development/geosoft_checks.xml"}


+def invalid_configuration(checkstyle_configs, use_spaces, indent_size):
+    return (checkstyle_configs == 'google' and
+        (not use_spaces or indent_size != 2))


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/bears/java/CheckstyleBear.py +++ b/bears/java/CheckstyleBear.py @@ -12,7 +12,7 @@ def invalid_configuration(checkstyle_configs, use_spaces, indent_size): return (checkstyle_configs == 'google' and - (not use_spaces or indent_size != 2)) + (not use_spaces or indent_size != 2)) def known_checkstyle_or_path(setting):

sils · 2016-10-25T23:16:06Z

bears/java/CheckstyleBear.py

+    if (checkstyle_configs is 'google' and
+            (not use_spaces or indent_size != 2)):
+        raise ValueError('Google checkstyle config does not support ' +
+                         'use_spaces=False or indent_size != 2')


what about the other configs? sun, geosoft, ...?

let's open another issue for that, at least we know from google that these settings don't fit together.

Makman2 · 2016-10-26T09:56:56Z

bears/java/CheckstyleBear.py

@@ -44,7 +51,8 @@ def setup_dependencies(self):

    def create_arguments(
            self, filename, file, config_file,
-            checkstyle_configs: known_checkstyle_or_path="google"):
+            checkstyle_configs: known_checkstyle_or_path="google",
+            use_spaces: bool=True, indent_size: int = 2):


spaces around second = :)

jayvdb · 2016-10-27T09:23:37Z

bears/java/CheckstyleBear.py

+def check_invalid_configuration(checkstyle_configs, use_spaces, indent_size):
+    if (checkstyle_configs is 'google' and
+            (not use_spaces or indent_size != 2)):
+        raise ValueError('Google checkstyle config does not support ' +


+ is not necessary or desirable here . Python automatically concatenates static string nodes in the AST.

Adrianzatreanu · 2016-10-29T09:09:50Z

bears/java/CheckstyleBear.py

@@ -10,6 +10,13 @@
    "geosoft": "http://geosoft.no/development/geosoft_checks.xml"}



regarding commit body: use "Adds" instead of "Add" (NOT in the shortlog, but in the body!)

also, end sentences with period please

sils · 2016-11-02T17:07:28Z

can you please use the full issue url, not just the number? Number onlky works in github, not on git locally.

sils · 2016-11-02T17:08:35Z

bears/java/CheckstyleBear.py

@@ -10,6 +10,13 @@
    "geosoft": "http://geosoft.no/development/geosoft_checks.xml"}


+def check_invalid_configuration(checkstyle_configs, use_spaces, indent_size):
+    if (checkstyle_configs is 'google' and


why plural? This is one config, not multiple... can you file an issue about renaming them or add another commit on top doing that?

sils · 2016-11-02T17:09:41Z

ok the code looks good, I want to get this merged. Also it seems like this has done quite a number of reviews already :/

One rebase, ping us and we'll merge it? Ideally correct the commit messsage with the issue URL.

Makman2 · 2016-11-05T19:06:28Z

@tylfin if you don't mind, could you make another rebase? sorry that this has gone so "long-term" :)

Adds invalid_config function to CheckstyleBear.py. Adds tests for invalid configurations. Closes coala#898.

sils · 2016-11-06T11:17:02Z

ack a5ce145

sils · 2016-11-06T11:17:08Z

@rultor merge

sils · 2016-11-06T11:17:41Z

Hey, this is your first contribution, right? Congrats and welcome to the coalaians! :)

You can help us a lot by filling out http://coala.io/newform!

Rultor will now automatically release your changes as a prerelease so users can experience the goodness of your changes right now.

See also https://github.com/coala-analyzer/coala/wiki/coala-Steam-Offer btw.

We would really appreciate it if you could help us with

some code reviews for other newcomers
fixing at least a difficulty/low issue

We're all volunteers and we're currently struggling to keep this up. Helping all those newcomers is a lot of work and we do need your help so we can continue this for others!

rultor · 2016-11-06T11:18:09Z

@rultor merge

@sils OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2016-11-06T11:19:33Z

@rultor merge

@sils Done! FYI, the full log is here (took me 1min)

gitmate-bot added process/pending review size/S review carefully! labels Oct 20, 2016

tylfin force-pushed the issue_898 branch from 6a010b9 to 383b35e Compare October 20, 2016 20:30

tylfin force-pushed the issue_898 branch from 383b35e to 6482629 Compare October 20, 2016 20:35

sils reviewed Oct 25, 2016

View reviewed changes

sils added process/wip and removed process/pending review labels Oct 25, 2016

tylfin force-pushed the issue_898 branch from 6482629 to 16f7e63 Compare October 25, 2016 17:31

gitmate-bot added process/pending review and removed process/wip labels Oct 25, 2016

gitmate-bot reviewed Oct 25, 2016

View reviewed changes

tylfin force-pushed the issue_898 branch from 16f7e63 to 52a7934 Compare October 25, 2016 17:33

Makman2 reviewed Oct 25, 2016

View reviewed changes

Makman2 added process/wip and removed process/pending review labels Oct 25, 2016

tylfin force-pushed the issue_898 branch from 52a7934 to 25aabda Compare October 25, 2016 18:48

gitmate-bot added process/pending review and removed process/wip labels Oct 25, 2016

gitmate-bot reviewed Oct 25, 2016

View reviewed changes

tylfin force-pushed the issue_898 branch from 25aabda to 68f0e94 Compare October 25, 2016 18:50

gitmate-bot reviewed Oct 25, 2016

View reviewed changes

sils reviewed Oct 25, 2016

View reviewed changes

Makman2 reviewed Oct 26, 2016

View reviewed changes

tylfin force-pushed the issue_898 branch from 8075a74 to ef8fd65 Compare October 26, 2016 13:43

jayvdb reviewed Oct 27, 2016

View reviewed changes

jayvdb mentioned this pull request Oct 27, 2016

CPPLintBear should error when use_spaces is False or indent_size is not 2 #896

Open

Adrianzatreanu reviewed Oct 29, 2016

View reviewed changes

Adrianzatreanu added process/wip and removed process/pending review labels Oct 29, 2016

tylfin force-pushed the issue_898 branch from ef8fd65 to 6ae6546 Compare November 1, 2016 23:28

gitmate-bot added process/pending review and removed process/wip labels Nov 1, 2016

sils reviewed Nov 2, 2016

View reviewed changes

sils added process/approved and removed process/pending review labels Nov 2, 2016

tylfin force-pushed the issue_898 branch from 6ae6546 to ceceeba Compare November 2, 2016 22:19

CheckstyleBear.py: Add configuration check

a5ce145

Adds invalid_config function to CheckstyleBear.py. Adds tests for invalid configurations. Closes coala#898.

tylfin force-pushed the issue_898 branch from ceceeba to a5ce145 Compare November 5, 2016 20:47

gitmate-bot added the process/pending review label Nov 5, 2016

sils removed the process/pending review label Nov 6, 2016

rultor merged commit a5ce145 into coala:master Nov 6, 2016

jayvdb mentioned this pull request Dec 23, 2016

CheckstyleBear docstring of mandatory settings #1192

Open

jeremydouglass mentioned this pull request May 20, 2017

Cache: inconsistent result in different runs based on the same coafile. #1750

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CheckstyleBear: Add invalid configuration check #917

CheckstyleBear: Add invalid configuration check #917

tylfin commented Oct 20, 2016

gitmate-bot commented Oct 20, 2016

gitmate-bot commented Oct 20, 2016

gitmate-bot commented Oct 20, 2016

aptrishu commented Oct 23, 2016 •

edited

Loading

sils commented Oct 25, 2016

sils Oct 25, 2016

sils Oct 25, 2016

gitmate-bot Oct 25, 2016

Makman2 Oct 25, 2016

Makman2 Oct 25, 2016

Makman2 Oct 25, 2016

tylfin Oct 25, 2016

jayvdb Oct 25, 2016

Makman2 Oct 25, 2016

Makman2 commented Oct 25, 2016

gitmate-bot Oct 25, 2016

gitmate-bot Oct 25, 2016

gitmate-bot Oct 25, 2016

gitmate-bot Oct 25, 2016

sils Oct 25, 2016

Makman2 Oct 26, 2016

Makman2 Oct 26, 2016

jayvdb Oct 27, 2016

Adrianzatreanu Oct 29, 2016

Adrianzatreanu Oct 29, 2016

sils commented Nov 2, 2016

sils Nov 2, 2016

sils commented Nov 2, 2016

Makman2 commented Nov 5, 2016

sils commented Nov 6, 2016

sils commented Nov 6, 2016

sils commented Nov 6, 2016

rultor commented Nov 6, 2016

rultor commented Nov 6, 2016

		@@ -10,6 +10,13 @@
		"geosoft": "http://geosoft.no/development/geosoft_checks.xml"}

CheckstyleBear: Add invalid configuration check #917

CheckstyleBear: Add invalid configuration check #917

Conversation

tylfin commented Oct 20, 2016

gitmate-bot commented Oct 20, 2016

gitmate-bot commented Oct 20, 2016

gitmate-bot commented Oct 20, 2016

aptrishu commented Oct 23, 2016 • edited Loading

sils commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Makman2 commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sils commented Nov 2, 2016

Choose a reason for hiding this comment

sils commented Nov 2, 2016

Makman2 commented Nov 5, 2016

sils commented Nov 6, 2016

sils commented Nov 6, 2016

sils commented Nov 6, 2016

rultor commented Nov 6, 2016

rultor commented Nov 6, 2016

aptrishu commented Oct 23, 2016 •

edited

Loading