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

Implement basic bear collection by aspect #4532

Merged
merged 5 commits into from Jul 24, 2017

Conversation

5 participants
@adhikasp
Member

adhikasp commented Jul 22, 2017

Closes #4531

@jayvdb

missing docstrings everywhere

tests/bearlib/aspects/CollectionsTest.py Outdated
self.unused_variable_leaves = AspectList([
Redundancy.UnusedVariable.UnusedGlobalVariable,
Redundancy.UnusedVariable.UnusedLocalVariable,
Redundancy.UnusedVariable.UnusedParameter

This comment has been minimized.

@jayvdb

jayvdb Jul 23, 2017

Member

trailing ,

coalib/settings/SectionFilling.py Outdated
bears,
[BEAR_KIND.LOCAL, BEAR_KIND.GLOBAL],
log_printer)
if hasattr(section, 'aspects') and section.aspects is not None:

This comment has been minimized.

@jayvdb

jayvdb Jul 23, 2017

Member

getattr(section, 'aspects', None) is one way to simplify this.

but IMO section.aspects should be set to None if aspects are not being used.

This comment has been minimized.

@adhikasp

adhikasp Jul 23, 2017

Member

The attrib checking is because of #4397 (comment). Some of tests is calling fill_settings directly without passing gather_configuration and that make section.aspects undeclared.

tests/test_bears/AspectTestBear.py Outdated
class AspectTestBear(LocalBear, aspects={
'detect': [get_aspect('unusedlocalvariable'),

This comment has been minimized.

@jayvdb

jayvdb Jul 23, 2017

Member

why isnt this something prettier like:

    'detect': AspectList([
        Redundancy.UnusedVariable.UnusedGlobalVariable,
        Redundancy.UnusedVariable.UnusedLocalVariable,
    ])
coalib/bearlib/aspects/base.py Outdated
@@ -75,6 +75,22 @@ def get_subaspect(parent, subaspect):
return child
def get_leaf_aspects(aspect):

This comment has been minimized.

@jayvdb

jayvdb Jul 23, 2017

Member

does this need to be a public function?

This comment has been minimized.

@adhikasp

adhikasp Jul 23, 2017

Member

No. Are you suggesting to declare it inside LeafAspectGetter class?

This comment has been minimized.

@jayvdb

jayvdb Jul 23, 2017

Member

I am suggesting you call it _get_leaf_aspects so that it is not public.

tests/bearlib/aspects/CollectionsTest.py Outdated
def test_get_leaf_aspects_duplicated_node(self):
aspects = AspectList([
Redundancy.UnusedVariable,
Redundancy.UnusedVariable.UnusedLocalVariable

This comment has been minimized.

@jayvdb

jayvdb Jul 23, 2017

Member

trailing ,

tests/bearlib/aspects/CollectionsTest.py Outdated
Metadata.CommitMessage.Shortlog.ColonExistence('py'),
Metadata.CommitMessage.Shortlog.FirstCharacter('py'),
Metadata.CommitMessage.Shortlog.Length('py'),
Metadata.CommitMessage.Shortlog.Tense('py')

This comment has been minimized.

@jayvdb

jayvdb Jul 23, 2017

Member

trailing , (everywhere!)

tests/bearlib/aspects/CollectionsTest.py Outdated
def test_get_leaf_aspects_unrelevant_exclude(self):
aspects = AspectList([Redundancy.UnusedVariable],
exclude=[Metadata]).get_leaf_aspects()

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 23, 2017

Collaborator

E128 continuation line under-indented for visual indent

Origin: PycodestyleBear (E128), Section: autopep8.

tests/bearlib/aspects/CollectionsTest.py Outdated
def test_get_leaf_aspects_unrelevant_exclude(self):
aspects = AspectList([Redundancy.UnusedVariable],
exclude=[Metadata]).get_leaf_aspects()

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 23, 2017

Collaborator

The code does not comply to PEP8.

Origin: PEP8Bear, Section: autopep8.

The issue can be fixed by applying the following patch:

--- a/tests/bearlib/aspects/CollectionsTest.py
+++ b/tests/bearlib/aspects/CollectionsTest.py
@@ -123,7 +123,7 @@
 
     def test_get_leaf_aspects_unrelevant_exclude(self):
         aspects = AspectList([Redundancy.UnusedVariable],
-            exclude=[Metadata]).get_leaf_aspects()
+                             exclude=[Metadata]).get_leaf_aspects()
 
         self.assertTrue(check_aspectlist_equal(
                         aspects, self.unused_variable_leaves))
@jayvdb

Aspect.remove needs lots more tests, and I suggest naming it _remove at the moment so that its design isnt critical at the moment and can be revised slightly.
It also needs :raises ...: in the docstring

tests/bearlib/aspects/CollectionsTest.py Outdated
self.assertCountEqual(aspects, self.unused_variable_leaves)
def test_get_leaf_aspects_unrelevant_exclude(self):

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

unrelevant is spelling mistake

tests/parsing/FilterTest.py Outdated
@@ -110,12 +110,12 @@ def test_filter_by_can_fix_null(self):
coala.main, 'coala', '-B', '--filter-by', 'can_fix')
self.assertEqual(retval, 0)
# 8 bears plus 1 line holding the closing colour escape sequence.
self.assertEqual(len(stdout.strip().splitlines()), 14)
self.assertEqual(len(stdout.strip().splitlines()), 15)

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

Please add a separate PR to put this number in a variable in tests.TestUtilities (#4545), so we dont need to change these lines all the time.
Ping maintainers to get it merged ASAP, so you can rebase this PR.

@@ -113,7 +113,7 @@ def test_version_conflict_in_collecting_bears(self, import_fn, _):
import_fn.side_effect = VersionConflict('msg1', 'msg2')
retval, stdout, stderr = execute_coala(
coala.main, 'coala', '--json', '-B')
self.assertEqual(retval, TEST_BEARS_COUNT)
self.assertEqual(retval, 13)

This comment has been minimized.

@adhikasp

adhikasp Jul 24, 2017

Member

I have no idea why this count doesn't go up like the rest.

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

That is a bug that you need to find.

This comment has been minimized.

@adhikasp

adhikasp Jul 24, 2017

Member

Because 13 here is not a bear count but an exit code https://github.com/coala/coala/blob/master/coalib/misc/Exceptions.py#L21

I mistakenly change this in prev PR because the number is same :(

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

ok, separate fixup commit please, with a descriptive commit message

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

done 👍

coalib/collecting/Collectors.py Outdated
bears_found = tuple([] for i in range(len(kinds)))
for aspect in aspects.get_leaf_aspects():
for bear in all_bears:
if (aspect in bear.aspects['detect'] or

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 24, 2017

Collaborator

Line contains following spacing inconsistencies:

  • Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/coalib/collecting/Collectors.py
+++ b/coalib/collecting/Collectors.py
@@ -218,7 +218,7 @@
     bears_found = tuple([] for i in range(len(kinds)))
     for aspect in aspects.get_leaf_aspects():
         for bear in all_bears:
-            if (aspect in bear.aspects['detect'] or 
+            if (aspect in bear.aspects['detect'] or
                     aspect in bear.aspects['fix']):
                 index = kinds.index(_get_kind(bear))
                 # Avoid duplicate
coalib/collecting/Collectors.py Outdated
bears_found = tuple([] for i in range(len(kinds)))
for aspect in aspects.get_leaf_aspects():
for bear in all_bears:
if (aspect in bear.aspects['detect'] or

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 24, 2017

Collaborator

W291 trailing whitespace

Origin: PycodestyleBear (W291), Section: autopep8.

coalib/collecting/Collectors.py Outdated
bears_found = tuple([] for i in range(len(kinds)))
for aspect in aspects.get_leaf_aspects():
for bear in all_bears:
if (aspect in bear.aspects['detect'] or

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 24, 2017

Collaborator

The code does not comply to PEP8.

Origin: PEP8Bear, Section: autopep8.

The issue can be fixed by applying the following patch:

--- a/coalib/collecting/Collectors.py
+++ b/coalib/collecting/Collectors.py
@@ -218,7 +218,7 @@
     bears_found = tuple([] for i in range(len(kinds)))
     for aspect in aspects.get_leaf_aspects():
         for bear in all_bears:
-            if (aspect in bear.aspects['detect'] or 
+            if (aspect in bear.aspects['detect'] or
                     aspect in bear.aspects['fix']):
                 index = kinds.index(_get_kind(bear))
                 # Avoid duplicate
@jayvdb

commit "coalaJSONTest: Revert wrong variable usage" should link to the relevant issue (even thought it is closed).

coalib/bearlib/aspects/base.py Outdated
leaf_aspects.append(aspect)
else:
search_leaf(aspect.subaspects.values())
search_leaf([aspect])

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

add a blank line above this.

coalib/bearlib/aspects/collections.py Outdated
for aspect in self:
if aspect is item or isinstance(aspect, item):
return super().remove(aspect)
raise ValueError('AspectList.remove(x): {} not in list.'.format(item))

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

add blank line above this

tests/test_bears/AspectTestBear.py Outdated
Root.Redundancy.UnusedVariable.UnusedGlobalVariable,
Root.Redundancy.UnusedVariable.UnusedLocalVariable,
]
}, languages=['Python']):

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

I find this indentation to be distracting. I think it would be better if aspects= appeared under LocalBear, ; and then languages= also appearing at the same indent level.
This might mean the length of the aspect class name is too long. if you need to, go back to using get_aspect to keep the line length neat.

This comment has been minimized.

@adhikasp

adhikasp Jul 24, 2017

Member

IMO that was weird too :/

I think this update is a bit better. Also I would like to avoid using get_aspect for better clarity at first glance about bear's aspect (the first time is because I was too lazy to search the correct fullname 😅 ).

coalib/bearlib/aspects/collections.py Outdated
Remove first matching item in list.
:param item: An aspectclass
:raises ValueError: When removed item is not found in list.

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

tense problem. it isnt a "removed item" because it hasnt been removed.

coalib/bearlib/aspects/collections.py Outdated
if aspect is item or isinstance(aspect, item):
return super().remove(aspect)
raise ValueError('AspectList.remove(x): {} not in list.'.format(item))

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

dont use literal AspectList ... it should be derived from the class object

coalib/bearlib/aspects/collections.py Outdated
:return: An AspectList contain ONLY leaf aspects.
"""
aspects = AspectList()

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

use type(self)

@@ -113,7 +113,7 @@ def test_version_conflict_in_collecting_bears(self, import_fn, _):
import_fn.side_effect = VersionConflict('msg1', 'msg2')
retval, stdout, stderr = execute_coala(
coala.main, 'coala', '--json', '-B')
self.assertEqual(retval, TEST_BEARS_COUNT)
self.assertEqual(retval, 13)

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

done 👍

tests/collecting/CollectorsTest.py Outdated
def test_aspect_bear(self):
with bear_test_module():
aspects = AspectList([get_aspect('unusedvariable')('py')])
local_bears, _ = collect_bears_by_aspects(

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

also get the global bears into a variable and assert that it is empty.

tests/test_bears/AspectTestBear.py Outdated
Root.Redundancy.UnusedVariable.UnusedGlobalVariable,
Root.Redundancy.UnusedVariable.UnusedLocalVariable,
]},
languages=['Python']):

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

put the ): on the next line, add a trailing ,, and dont have a blank line between the ): and the first line of code.

tests/test_bears/AspectTestBear.py Outdated
def run(self, filename, file, config: str=''):
"""
Bear that have aspect.
:param config: An optional dummy config file.

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

blank line between summary and parameters.

tests/test_bears/AspectTestBear.py Outdated
message='This is just a dummy result',
severity=RESULT_SEVERITY.INFO,
file=filename,
aspect=Root.Redundancy.UnusedVariable.UnusedLocalVariable)

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

trailing , and move ) to the next line.

coalib/collecting/Collectors.py Outdated
@@ -204,6 +204,31 @@ def filter_section_bears_by_languages(bears, languages):
return new_bears
def collect_bears_by_aspects(aspects, kinds):
"""
Collect list of bears that have capability to analyze all aspects from

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

One line summary first. then blank line and more detailed summary. see PEP 257

coalib/bearlib/aspects/base.py Outdated
@@ -86,6 +109,18 @@ def __get__(self, obj, owner):
return functools.partial(get_subaspect, parent)
class LeafAspectGetter:
"""
Special "getter" class to implement ``get_leaf_aspects()`` method in

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

PEP 257

This comment has been minimized.

@adhikasp

adhikasp Jul 24, 2017

Member

Umm.. are you referring to "single line summary first"?

coalib/bearlib/aspects/base.py Outdated
@@ -75,6 +75,29 @@ def get_subaspect(parent, subaspect):
return child
def _get_leaf_aspects(aspect):
"""
Explode an arbitrary aspect into list of its leaf aspects.

This comment has been minimized.

@jayvdb

jayvdb Jul 24, 2017

Member

arbitrary here adds no extra meaning.

@jayvdb

jayvdb approved these changes Jul 24, 2017

@jayvdb

This comment has been minimized.

Member

jayvdb commented Jul 24, 2017

ack 497e0c4 6734d86 31304ac 8f750d4 bafc09e

adhikasp added some commits Jul 13, 2017

Test: Add AspectTestBear
Add new bear for any test that require aspect bear.
aspects: Create ``get_leaf_aspects`` method
``get_leaf_aspects()`` method is used for "breaking down" arbitrary
aspect into its leaf aspect(s). The objective is to make it easier
for the bear collector to match individual leaf aspect with a capable
bear.
Collectors: Create basic bear collector by aspect
Enable coala to collect bears based on aspects configuration.
Currently, the algorithm to collects is very simple, iterate
over all bears in coala and pick the first bear that capable to
handle an aspect and have compatible language. Improvement will
be implemented in future patch.

Closes #4531
Bear.py: Make ``languages`` JSON compliant
``languages`` attribute have type of ``Language`` and cannot
automatically converted into JSON object. This explicitly
tell to convert Language to string when converting Bear
object to JSON.
coalaJSONTest: Revert wrong variable usage
Revert the change from commit 07092f2
which mistakenly change a constant exit code value assertion when
running coala with VersionConflict error with TEST_BEARS_COUNT
constant.

Fixes #4551
@jayvdb

This comment has been minimized.

Member

jayvdb commented Jul 24, 2017

@Nosferatul

This comment has been minimized.

Member

Nosferatul commented Jul 24, 2017

@jayvdb this is ready for merge 👍

@jayvdb

This comment has been minimized.

Member

jayvdb commented Jul 24, 2017

@rultor merge

@rultor

This comment has been minimized.

Contributor

rultor commented Jul 24, 2017

@rultor merge

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

@rultor rultor merged commit 4f8ee34 into coala:master Jul 24, 2017

6 of 9 checks passed

ci/circleci Your tests are queued behind your running builds
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
codecov/project 100% (target 100%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
review/gitmate/commit This commit has no issues. :)
Details
review/gitmate/manual This commit was acknowledged.
Details
review/gitmate/pr This PR has no issues. :)
Details
@rultor

This comment has been minimized.

Contributor

rultor commented Jul 24, 2017

@rultor merge

@jayvdb Done! FYI, the full log is here (took me 2min)

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