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

AspectList: Add ``exclude`` attribute #4439

Merged
merged 1 commit into from Jul 13, 2017

Conversation

@adhikasp
Member

adhikasp commented Jul 2, 2017

Extend AspectList class to have excludes attribute that hold list of
aspect that marked as exception of the list. Trying to access aspect
or subaspect of item in excludes will always give False/None.

>>> Metadata not in AspectList([Root], excludes=[Metadata])
True
>>> AspectList([Root], excludes=[Metadata]).get('metadata')
None

This feature will be used to excluding ("deactivate") an aspect that
was defined in user's coala configuration.

Closes #4438

Note that this PR is dependent (and blocked) by #4427 and contain commit from that PR too.

tests/bearlib/aspects/CollectionsTest.py Outdated
self.assertIsNone(self.instancelist_excludes.get(
Metadata.CommitMessage.Body.Existence))

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 2, 2017

Collaborator

W391 blank line at end of file'

PycodestyleBear (W391), severity NORMAL, section autopep8.

tests/bearlib/aspects/CollectionsTest.py Outdated
Metadata.CommitMessage.Shortlog.TrailingPeriod))
self.assertIsNone(self.instancelist_excludes.get(
Metadata.CommitMessage.Body.Existence))

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 2, 2017

Collaborator

The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/tests/bearlib/aspects/CollectionsTest.py
+++ b/tests/bearlib/aspects/CollectionsTest.py
@@ -90,5 +90,3 @@
                           Metadata.CommitMessage.Shortlog.TrailingPeriod))
         self.assertIsNone(self.instancelist_excludes.get(
                           Metadata.CommitMessage.Body.Existence))
-
-
coalib/bearlib/aspects/collections.py Outdated
def __contains__(self, aspect):
for item in self:
if issubaspect(aspect, item):
return True
return not aspect in self.excludes

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 2, 2017

Collaborator

E713 test for membership should be 'not in''

PycodestyleBear (E713), severity NORMAL, section autopep8.

@jayvdb

You need to add a test for for aspect in aspectlist (using assertIn & assertNotIn, and I suspect it will fail at the moment.

tests/bearlib/aspects/CollectionsTest.py Outdated
def setUp(self):
self.aspectlist_excludes = AspectList(
[Metadata.CommitMessage.Shortlog, Metadata.CommitMessage.Body],
[Metadata.CommitMessage.Shortlog.TrailingPeriod,

This comment has been minimized.

@jayvdb

jayvdb Jul 6, 2017

Member

use excludes= so it is more readable.

tests/bearlib/aspects/CollectionsTest.py Outdated
self.instancelist_excludes = AspectList(
[Metadata.CommitMessage.Shortlog('py'),
Metadata.CommitMessage.Body('py')],
[Metadata.CommitMessage.Shortlog.TrailingPeriod,

This comment has been minimized.

@jayvdb

jayvdb Jul 6, 2017

Member

use excludes= so it is more readable.

@userzimmermann

Hey @adhikasp :) Nice work! But better name the argument exclude instead of excludes. The imperative form is just more common

And please put the changes to coalib/bearlib/aspects/base.py into a separate PR, since these are not related to AspectList

cc @jayvdb

coalib/bearlib/aspects/collections.py Outdated
@@ -21,14 +21,19 @@ def __init__(self, seq=()):
:param seq: A sequence containing either aspectclass, aspectclass
instance, or string of partial/full qualified aspect name.
:param excludes: A sequence of either aspectclass, aspectclass
instance, or aspect name that marked as excluded

This comment has been minimized.

@userzimmermann

userzimmermann Jul 6, 2017

Member

aspect instances make no sense here

coalib/bearlib/aspects/base.py Outdated
NotImplementedError because of instanced aspect doesn't instance
its children and will cause wrong result.
See https://github.com/coala/coala/issues/4388
We can also get child instance of an aspect instance.

This comment has been minimized.

@userzimmermann

userzimmermann Jul 6, 2017

Member

This is not related to this PR

coalib/bearlib/aspects/base.py Outdated
Traceback (most recent call last):
...
NotImplementedError: Cannot access children of aspect instance.
<...CommitMessage object at 0x...>

This comment has been minimized.

@userzimmermann

userzimmermann Jul 6, 2017

Member

This is also not related to this PR

coalib/bearlib/aspects/base.py Outdated
@@ -63,9 +58,6 @@ def get_subaspect(parent, subaspect):
if isinstance(subaspect, aspectbase):
raise AttributeError('Cannot search an aspect instance using '
'another aspect instance as argument.')
if isinstance(parent, aspectbase):
raise NotImplementedError('Cannot access children of aspect '
'instance.')

This comment has been minimized.

@userzimmermann

userzimmermann Jul 6, 2017

Member

Even this is not related to this PR

coalib/bearlib/aspects/base.py Outdated
instanced_child = {}
for name, child in self.subaspects.items():
instanced_child[name] = child(language, **taste_values)
self.__dict__['subaspects'] = instanced_child

This comment has been minimized.

@userzimmermann

userzimmermann Jul 6, 2017

Member

And this is definitely not related to this PR

coalib/bearlib/aspects/collections.py Outdated
@@ -7,7 +7,7 @@ class AspectList(list):
List-derived container to hold aspects.
"""
def __init__(self, seq=()):
def __init__(self, seq=(), excludes=()):

This comment has been minimized.

@userzimmermann

userzimmermann Jul 6, 2017

Member

Change argument defaults to None. Otherwise you always do checks like if excludes .... But there can be sequence that are False but can anyway contain elements. Best is always seq=None, excludes=None and then check if excludes is None .... It's also written in some python docs...

This comment has been minimized.

@adhikasp

adhikasp Jul 6, 2017

Member

Agree for the exclude=None. But if we change seq=None then it could give an error for declaring empty AspectList.

>>> from coalib.bearlib.aspects.collections import AspectList
>>> AspectList()
Traceback (most recent call last):
    ...
TypeError: 'NoneType' object is not iterable

@userzimmermann userzimmermann added this to the #aspectsYEAH milestone Jul 6, 2017

@userzimmermann userzimmermann changed the title from AspectList: Add ``excludes`` attribute to [WIP] AspectList: Add ``excludes`` attribute Jul 6, 2017

@userzimmermann

This comment has been minimized.

Member

userzimmermann commented Jul 6, 2017

@adhikasp @jayvdb Just saw that the commit with changes to coalib/bearlib/aspects/base.py is actually taken from #4427 ... Sorry for complaining! But I marked this extra [WIP] ;)

@jayvdb jayvdb changed the title from [WIP] AspectList: Add ``excludes`` attribute to AspectList: Add ``exclude`` attribute Jul 12, 2017

@jayvdb

This comment has been minimized.

Member

jayvdb commented Jul 12, 2017

@userzimmermann , the commit for #4427 is a dependency for #4438 , and this was explained in the top comment of this PR.

It is sensible to have one PR that allows them to be reviewed together, provided that the commits are nice and cleanly separated.

tests/bearlib/aspects/CollectionsTest.py Outdated
self.assertEqual(self.instancelist_excludes.get(
Metadata.CommitMessage.Shortlog.ColonExistence),
Metadata.CommitMessage.Shortlog.ColonExistence('py'))

This comment has been minimized.

@jayvdb

jayvdb Jul 13, 2017

Member

Put this computed value in a variable before using assertEqual.
assert invocations shouldnt have complex calls in them.

This comment has been minimized.

@adhikasp

adhikasp Jul 13, 2017

Member

I don't think this is a complex call because it just a method call with 1 arg (albeit with very long names). Also every other test have the same pattern.

But I'll try shorten the aspect name to make it easier to read.

tests/bearlib/aspects/CollectionsTest.py Outdated
CommitMessage.Shortlog.TrailingPeriod))
self.assertIsNone(self.instancelist_excludes.get(
CommitMessage.Body.Existence))

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 13, 2017

Collaborator

W391 blank line at end of file'

PycodestyleBear (W391), severity NORMAL, section autopep8.

tests/bearlib/aspects/CollectionsTest.py Outdated
CommitMessage.Shortlog.TrailingPeriod))
self.assertIsNone(self.instancelist_excludes.get(
CommitMessage.Body.Existence))

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 13, 2017

Collaborator

The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/tests/bearlib/aspects/CollectionsTest.py
+++ b/tests/bearlib/aspects/CollectionsTest.py
@@ -92,4 +92,3 @@
                           CommitMessage.Shortlog.TrailingPeriod))
         self.assertIsNone(self.instancelist_excludes.get(
                           CommitMessage.Body.Existence))
-
@jayvdb

jayvdb approved these changes Jul 13, 2017

AspectList: Add ``exclude`` attribute
Extend AspectList class to have `exclude` attribute that hold list of
aspect that marked as exception of the list. Trying to access aspect
or subaspect of item in `exclude` will always give False/None.

```.python
>>> Metadata not in AspectList([Root], exclude=[Metadata])
True
>>> AspectList([Root], exclude=[Metadata]).get('metadata')
None
```

This feature will be used to excluding an aspect (to "deactivate") that
was defined in user's coala configuration.

Closes #4438
@jayvdb

This comment has been minimized.

Member

jayvdb commented Jul 13, 2017

ack 25bea08

@jayvdb

This comment has been minimized.

Member

jayvdb commented Jul 13, 2017

@rultor merge

@rultor

This comment has been minimized.

Contributor

rultor commented Jul 13, 2017

@rultor merge

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

@rultor rultor merged commit 25bea08 into coala:master Jul 13, 2017

6 of 9 checks passed

ci/circleci CircleCI is running your tests
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 No issues with this one - go ahead! :)
Details
review/gitmate/manual This commit was acknowledged.
Details
review/gitmate/pr All is well! :) (0 problems solved)
Details
@rultor

This comment has been minimized.

Contributor

rultor commented Jul 13, 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