Add review bot tool for checkstyle

Review Request #9488 — Created Jan. 16, 2018 and submitted

Information

ReviewBot
master
b53ebe9...

Reviewers

Checkstyle is a development tool to help developers write
Java code that adheres to a coding standard. It automates
the process of checking Java code.
This makes it ideal for projects that want to enforce a
coding standard.

Checkstyle can check many aspects of the source code.
It can find class design problems, method design problems.
It also has the ability to check code layout and formatting
issues.

http://checkstyle.sourceforge.net/

Added checkstyle as tool and saw that it finds correct "errors".

Description From Last Updated

Your description needs to be fleshed out a bit. Please see https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea for details.

daviddavid

Some of the older files may have this at the top, but we don't duplicate this in each file anymore. …

daviddavid

I think you meant to write "Review Bot tool"

daviddavid

There may be many worker nodes, which may not all have the same config file present. Can we instead define …

daviddavid

There's an extra > at the end of the line. This should also likely be "Checkstyle configuration file", though if …

daviddavid

If there's some error when executing, will it give us valid XML to parse? Can this raise an exception that …

daviddavid

Can we call this checkstyle_jar_path (or even just checkstyle_path) for consistency with the PMD config value?

daviddavid

pass is unnecessary if there's another statement in the except handler.

daviddavid

Should this be "checkers for java code"?

daviddavid

Please wrap to 80 columns.

daviddavid

Please wrap to 80 columns.

daviddavid
misery
misery
  1. 
      
  2. checkstyle has a check for naming convention of java files. It checks if a class uses the same name for the filename. As the reviewbot create a temporary filename it will be an error for each run.

    "The name of the outer type and the file do not match."

    Is there an option to use a temporary directory with the real filename?

    1. Ok, same problem with "package name". It needs a tempdir with the same source dir structure.
      Package name is not same as directory.

  3. 
      
misery
misery
david
  1. 
      
  2. Your description needs to be fleshed out a bit. Please see https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea for details.

  3. bot/reviewbot/tools/checkstyle.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Some of the older files may have this at the top, but we don't duplicate this in each file anymore. If your company needs to claim copyright that can go in the COPYING file.

  4. bot/reviewbot/tools/checkstyle.py (Diff revision 4)
     
     

    I think you meant to write "Review Bot tool"

  5. bot/reviewbot/tools/checkstyle.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     

    There may be many worker nodes, which may not all have the same config file present. Can we instead define the configuration XML in the options and write that out to a file while executing?

    1. Yeah, ok... but a checkstyle config xml can be up to more than 5 or 10 kb! Is it possible to upload a file to the settings or use a textarea?

      By the way.... same problem is with pmd. Should be the same behaviour.

    2. You should be able to do a texarea by having:

      'field_options': {
          'widget': 'django.forms.Textarea',
          ...
      },
      
    3. Alternatively, I wonder if it'd be useful to have two options here:

      1. Provide raw XML text content to be written to disk.
      2. Provide a URI to an XML file somewhere. If that's locally on disk, the URI will be file://. If it's remote, https://, etc.

      Just an idea, but perhaps out of scope for now.

    4. I tried 'widget': 'django.forms.Textarea' but got no field anymore. Also I looked into JSHint checker and tried those. But it isn't working, too. Maybe JSHint is broken? I cannot see any textarea there.

    5. Ok, my mistake.... fixed. :-)

      @Mike
      Yeah, nice idea. But that can be another changeset. :-)

  6. bot/reviewbot/tools/checkstyle.py (Diff revision 4)
     
     

    There's an extra > at the end of the line. This should also likely be "Checkstyle configuration file", though if you make the change to have this be the config XML you will want to change the label/help text to match.

  7. bot/reviewbot/tools/checkstyle.py (Diff revision 4)
     
     

    If there's some error when executing, will it give us valid XML to parse? Can this raise an exception that should be caught?

  8. 
      
misery
misery
david
  1. 
      
  2. bot/reviewbot/config.py (Diff revision 6)
     
     

    Can we call this checkstyle_jar_path (or even just checkstyle_path) for consistency with the PMD config value?

  3. bot/reviewbot/tools/checkstyle.py (Diff revision 6)
     
     

    pass is unnecessary if there's another statement in the except handler.

  4. Should this be "checkers for java code"?

  5. Please wrap to 80 columns.

    1. uh, yeah, you should enable doc8 if it is merged. :-)

  6. Please wrap to 80 columns.

  7. 
      
misery
misery
david
  1. Ship It!
  2. 
      
misery
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (29282ba)
Loading...