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

GoMetaLintBear: Add bear implementation #2335

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .ci/deps.sh
Expand Up @@ -20,6 +20,7 @@ go get -u sourcegraph.com/sqs/goreturns
go get -u golang.org/x/tools/cmd/gotype
go get -u github.com/kisielk/errcheck
go get -u github.com/BurntSushi/toml/cmd/tomlv
go get -u github.com/alecthomas/gometalinter
Copy link
Member

Choose a reason for hiding this comment

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

It is better to install stable version, see https://github.com/alecthomas/gometalinter#installing


# Ruby commands
bundle install --path=vendor/bundle --binstubs=vendor/bin --jobs=8 --retry=3
Expand Down Expand Up @@ -62,3 +63,6 @@ wget "https://downloads.sourceforge.net/project/astyle/astyle/astyle%203.0.1/ast
tar -xvzf ~/astyle.tar.gz -C ~/
make -C ~/astyle/build/gcc
sudo make install -C ~/astyle/build/gcc

# gometalinter installation
gometalinter --install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is to be added here, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

Copied this to #2536

83 changes: 83 additions & 0 deletions bears/go/GoMetaLintBear.py
@@ -0,0 +1,83 @@
from coalib.bearlib.abstractions.Linter import linter
from dependency_management.requirements.GoRequirement import GoRequirement
from coalib.results.RESULT_SEVERITY import RESULT_SEVERITY
from coalib.settings.Setting import typed_list


@linter(executable='gometalinter',
output_format='regex',
output_regex=r'\w+\.go:'
Copy link
Member

Choose a reason for hiding this comment

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

Invalid regex, the regex mentioned in their repo is <file>:<line>:[<column>]: <message> (<linter>)

r'(?P<line>\d+):'
r'(?P<column>\d*):'
r'(?P<severity>[a-z]+): '
r'(?P<message>.*) '
r'(?P<additional_info>\([a-z]+\))',
severity_map={'error': RESULT_SEVERITY.MAJOR,
'warning': RESULT_SEVERITY.NORMAL})
class GoMetaLintBear:
"""
Lints your Go files!
Concurrently runs a number of Go lint tools.
"""

LANGUAGES = {'Go'}
REQUIREMENTS = {
GoRequirement(package='github.com/alecthomas/gometalinter', flag='-u')}
AUTHORS = {'The coala developers'}
AUTHORS_EMAILS = {'coala-devel@googlegroups.com'}
LICENSE = 'AGPL-3.0'
SEE_MORE = 'http://gopkg.in/alecthomas/gometalinter.v2'

def create_arguments(self, filename, file, config_file,
enable_checks: typed_list(str) = (),
enable_all_checks: bool = False,
disable_checks: typed_list(str) = (),
disable_all_checks: bool = False,
gometalinter_config_file: str = '',
):
"""
:param enable_checks:
Copy link
Member

Choose a reason for hiding this comment

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

bear specific settings should use names which are very unambiguous and not re-usable across bears. enable_checks and others are not clear which bear they belong to.

Also checks still feels wrong, as the values in them are linters / commands , not specific checks.

To modify an individual linter, it needs to be overriden using --linter:... as mentioned at https://github.com/alecthomas/gometalinter#adding-custom-linters .

We have an ongoing problem with how we handle finite lists like this.
https://github.com/coala/coala-bears/pull/2390/files#diff-565b15f3772ba98b4bcaa248fd442350R1 is one approach.
#2431 shows some of the problems we caused ourselves by having a default list.
For further complexity caused by these settings, see https://github.com/coala/cEPs/blob/master/cEP-0005.md and coala/cEPs#139 .

So I think the setting should be called gometa_linters , and it should support value * which makes a separate setting for enable_all_checks unnecessary.

When gometa_linters has any value other than * , it would always use --disable_all_checks so that the select list is always the exact list which will be enabled, and not additive to an existing set of linters that coala has no knowledge of.

A ignore list (disable_checks) would then be unnecessary as the user can always explicitly list the linters they do want.
That setting value might get very long, but it is at least very clear what is happening.

If you are still undecided on how to approach these settings, the best approach is to backup this branch, and remove these config settings for the first merge, which would mean the user has two options : use gometalinter's default settings or create a config gometalinter file and do any combination they like. Not perfect, but not limited either. Then we can work more on these settings in a follow up issue.

List of linters to enable. Some linters are disabled by default.
Refer https://github.com/alecthomas/gometalinter#supported-linters
:param enable_all_checks:
Enable all supported linters.
:param disable_checks:
List of linters to disable.
:param disable_all_checks:
Disable all supported linters.
:param gometalinter_config_file:
A JSON configuration file for gometalinter.
It overrides ``.gometalinter.json`` which is picked up by
default, if present. Provide ``False`` in order to prevent this
default pickup behavior.
"""
# Arguments are parsed in order.
args = (filename,)

if (enable_checks and enable_all_checks) or (
disable_checks and disable_all_checks):
self.err('The arguments you provided are mutually exclusive.')
return

if enable_checks and disable_all_checks:
args += ('--disable-all',)
args += tuple('--enable={}'.format(e) for e in enable_checks)
elif disable_all_checks:
args += ('--disable-all',)
elif enable_checks:
args += tuple('--enable={}'.format(e) for e in enable_checks)

if disable_checks and enable_all_checks:
args += ('--enable-all',)
args += tuple('--disable={}'.format(e) for e in disable_checks)
elif enable_all_checks:
args += ('--enable-all',)
elif disable_checks:
args += tuple('--disable={}'.format(d) for d in disable_checks)

if gometalinter_config_file == 'False':
args += ('--no-config',)
elif gometalinter_config_file:
args += ('--config={}'.format(gometalinter_config_file),)

return args
117 changes: 117 additions & 0 deletions tests/go/GoMetaLintBearTest.py
@@ -0,0 +1,117 @@
import os

from bears.go.GoMetaLintBear import GoMetaLintBear
from queue import Queue
from unittest.case import skipIf
from shutil import which

from coalib.testing.LocalBearTestHelper import execute_bear
from coalib.testing.LocalBearTestHelper import LocalBearTestHelper
from coalib.results.Result import Result
from coalib.results.RESULT_SEVERITY import RESULT_SEVERITY
from coalib.settings.Section import Section
from coalib.settings.Setting import Setting


def get_test_file_path(testfile):
return os.path.join(os.path.dirname(__file__),
'gometalinter_test_files',
testfile)


def load_test_file(filename):
with open(get_test_file_path(filename)) as file:
contents = file.read().splitlines(True)
return contents


@skipIf(which('gometalinter') is None, 'gometalinter is not installed')
class GoMetaLintBearTest(LocalBearTestHelper):

def setUp(self):
self.section = Section('')
self.uut = GoMetaLintBear(self.section, Queue())
self.bad_testfile1 = get_test_file_path('bad_testfile1.go')
self.bad_testfile1_contents = load_test_file('bad_testfile1.go')
self.bad_testfile2 = get_test_file_path('bad_testfile2.go')
self.bad_testfile2_contents = load_test_file('bad_testfile2.go')
self.good_testfile = get_test_file_path('good_testfile.go')
self.config_file = get_test_file_path('test_config.json')
self.maxDiff = None

def test_validity(self):
self.check_validity(self.uut, [], self.good_testfile)
self.check_invalidity(self.uut, [], self.bad_testfile1)

def test_mutually_exclusive_settings(self):
self.section.append(Setting('enable_checks', 'golint,vet'))
self.section.append(Setting('enable_all_checks', 'True'))
with execute_bear(self.uut, 'bad_testfile2.go',
self.bad_testfile2_contents) as results:
self.assertEqual(len(results), 0)

def test_disable_all_and_enable(self):
self.check_results(self.uut,
self.bad_testfile1_contents,
[Result.from_values(
'GoMetaLintBear',
message='undeclared name: fmt',
file=self.bad_testfile1,
line=4,
column=4,
additional_info='(gotype)',
severity=RESULT_SEVERITY.MAJOR)],
filename=self.bad_testfile1,
settings={'disable_all_checks': True,
'enable_checks': ['gotype']})

def test_disable_all(self):
self.section.append(Setting('disable_all_checks', 'True'))
self.check_validity(self.uut, [], self.bad_testfile1)

def test_enable(self):
self.section.append(Setting('enable_checks', 'gotype,vet,structcheck'))
self.check_invalidity(self.uut, [], self.bad_testfile2)

def test_enable_all_and_disable(self):
self.check_results(self.uut,
self.bad_testfile2_contents,
[],
filename=self.bad_testfile2,
settings={'enable_all_checks': True,
'disable_checks': ['golint']})

def test_enable_all(self):
self.check_results(self.uut,
self.bad_testfile2_contents,
[Result.from_values(
'GoMetaLintBear',
message='should omit type int from '
'declaration of var x; it '
'will be inferred from the '
'right-hand side',
file=self.bad_testfile2,
line=6,
column=10,
additional_info='(golint)',
severity=RESULT_SEVERITY.NORMAL)],
filename=self.bad_testfile2,
settings={'enable_all_checks': True})

def test_disable(self):
self.section.append(Setting('disable_checks', 'golint'))
self.check_validity(self.uut, [], self.bad_testfile2)

def test_config_file(self):
self.section.append(
Setting('gometalinter_config_file', 'test_config.json'))
self.check_validity(self.uut, [], self.bad_testfile2)

def test_ignore_config_file(self):
self.section.append(Setting('gometalinter_config_file', 'False'))
self.check_results(self.uut,
self.bad_testfile2_contents,
[],
filename=self.bad_testfile2,
settings={'disable_all_checks': True,
'enable_checks': ['gotype', 'vet']})
5 changes: 5 additions & 0 deletions tests/go/gometalinter_test_files/bad_testfile1.go
@@ -0,0 +1,5 @@
package main

func main() {
fmt.Println("GoMetaLinter")
}
8 changes: 8 additions & 0 deletions tests/go/gometalinter_test_files/bad_testfile2.go
@@ -0,0 +1,8 @@
package main

import "fmt"

func main() {
var x int = 10
fmt.Println(x)
}
7 changes: 7 additions & 0 deletions tests/go/gometalinter_test_files/good_testfile.go
@@ -0,0 +1,7 @@
package main

import "fmt"

func main() {
fmt.Println("GoMetaLinter")
}
3 changes: 3 additions & 0 deletions tests/go/gometalinter_test_files/test_config.json
@@ -0,0 +1,3 @@
{
"Disable": ["golint", "gotype", "vet", "vetshadow"]
Copy link
Member

Choose a reason for hiding this comment

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

package.json uses 2 space indent.
could we use that here also?

The example at https://github.com/alecthomas/gometalinter#configuration-file also uses two spaces.

}