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.

Sign up for GitHub

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

Jump to bottom

AspectList: Overload __init__ to accept strings #4389

Merged
merged 1 commit into from
Jun 25, 2017
Merged

AspectList: Overload __init__ to accept strings #4389

merged 1 commit into from
Jun 25, 2017

Conversation

adhikasp
Copy link
Member

Overload aspectlist so it could be instanced from a list of string
containing aspects name. The reason of this overloading is to make
it possible to instanced list of desired aspects found in coafile.

>>> aspectlist(['coalaCorrect', 'aspectsYEAH'])
[<aspectclass '...coalaCorrect'>, <aspectclass '...aspectsYEAH'>]

Replacement of #4379
Closes #4382

gitmate-bot
gitmate-bot reviewed Jun 22, 2017
View reviewed changes
coalib/bearlib/aspects/collections.py Outdated
@@ -1,3 +1,4 @@
import coalib.bearlib.aspects
Copy link
Collaborator

Choose a reason for hiding this comment

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

W291 trailing whitespace'

PycodestyleBear (W291), severity NORMAL, section autopep8.

gitmate-bot
gitmate-bot reviewed Jun 22, 2017
View reviewed changes
coalib/bearlib/aspects/collections.py Outdated
@@ -1,3 +1,4 @@
import coalib.bearlib.aspects
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/coalib/bearlib/aspects/collections.py
+++ b/coalib/bearlib/aspects/collections.py
@@ -1,4 +1,4 @@
-import coalib.bearlib.aspects 
+import coalib.bearlib.aspects
 from coalib.bearlib.aspects.meta import issubaspect, assert_aspect

gitmate-bot
gitmate-bot reviewed Jun 22, 2017
View reviewed changes
coalib/bearlib/aspects/collections.py Outdated
@@ -1,3 +1,4 @@
import coalib.bearlib.aspects
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Trailing whitespaces.

SpaceConsistencyBear, severity NORMAL, section python.

The issue can be fixed by applying the following patch:

--- a/coalib/bearlib/aspects/collections.py
+++ b/coalib/bearlib/aspects/collections.py
@@ -1,4 +1,4 @@
-import coalib.bearlib.aspects 
+import coalib.bearlib.aspects
 from coalib.bearlib.aspects.meta import issubaspect, assert_aspect

@adhikasp adhikasp force-pushed the adhikasp/aspectlist-overload branch from eef77c2 to 62093d6 Compare June 22, 2017 16:33
@adhikasp adhikasp mentioned this pull request Jun 22, 2017
Closed
@userzimmermann userzimmermann added this to the #aspectsYEAH milestone Jun 22, 2017
userzimmermann
userzimmermann requested changes Jun 22, 2017
View reviewed changes
Copy link
Member

@userzimmermann userzimmermann left a comment

Choose a reason for hiding this comment

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

@adhikasp Good approach but doesn't handle mixed cases :/

coalib/bearlib/aspects/collections.py Outdated
instance, or string of partial/full qualified aspect name.
"""
if all(map((lambda x: isinstance(x, str)), seq)):
seq = map((lambda x: coalib.bearlib.aspects[x]), seq)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't cover mixed cases like aspectlist([Root.Spelling.coalaCorrect, 'aspectsYEAH'])

Better super().__init__(item if isaspect(item) else coalib.bearlib.aspects[item] for item in seq)

tests/bearlib/aspects/CollectionsTest.py Outdated
@@ -9,10 +9,15 @@
class aspectlistTest:

def test__init__(self):
with pytest.raises(aspectTypeError) as exc:
list_of_aspect = aspectlist(['CommitMessage.Shortlog',
'CommitMessage.Body'])
Copy link
Member

Choose a reason for hiding this comment

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

Test aspectclass/instance/string-only and mixed cases

coalib/bearlib/aspects/collections.py Outdated
>>> aspectlist([Root.Spelling.coalaCorrect, Root.Spelling.aspectsYEAH])
[<aspectclass '...coalaCorrect'>, <aspectclass '...aspectsYEAH'>]
>>> aspectlist(['coalaCorrect', 'aspectsYEAH'])
[<aspectclass '...coalaCorrect'>, <aspectclass '...aspectsYEAH'>]
Copy link
Member

Choose a reason for hiding this comment

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

Also add mixed cases

userzimmermann
userzimmermann requested changes Jun 22, 2017
View reviewed changes
Copy link
Member

@userzimmermann userzimmermann left a comment

Choose a reason for hiding this comment

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

And please add Relates to https://gitlab.com/coala/GSoC-2017/issues/299 to the commit message

@adhikasp adhikasp force-pushed the adhikasp/aspectlist-overload branch from 62093d6 to a530545 Compare June 22, 2017 19:33
@adhikasp
Copy link
Member Author

@userzimmermann updated

@adhikasp adhikasp force-pushed the adhikasp/aspectlist-overload branch 2 times, most recently from dd480b6 to 497adf7 Compare June 23, 2017 21:31
@adhikasp adhikasp mentioned this pull request Jun 24, 2017
Merged
@adhikasp adhikasp force-pushed the adhikasp/aspectlist-overload branch from 497adf7 to b8a7d27 Compare June 24, 2017 19:06
jayvdb
jayvdb reviewed Jun 24, 2017
View reviewed changes
tests/bearlib/aspects/CollectionsTest.py Outdated
aspectlist(['String'])
exc.match("'String' is not an aspectclass or "
'an instance of an aspectclass')
exc.match("no aspect named 'String'")
Copy link
Member

Choose a reason for hiding this comment

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

exception messages should begin with a capital letter: No aspect named ...

Please raise a bug to fix that.

Copy link
Member

Choose a reason for hiding this comment

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

I've raised #4402

jayvdb
jayvdb reviewed Jun 25, 2017
View reviewed changes
coalib/bearlib/aspects/collections.py Outdated

>>> from .root import Root
>>> from .Spelling import Spelling
>>> aspectlist([Root.Spelling.coalaCorrect, Root.Spelling.aspectsYEAH])
Copy link
Member

Choose a reason for hiding this comment

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

#4401 should be fixed first, otherwise lots of unnecessary changes occur.

@adhikasp adhikasp force-pushed the adhikasp/aspectlist-overload branch from b8a7d27 to c6cb0bb Compare June 25, 2017 13:21
@adhikasp adhikasp force-pushed the adhikasp/aspectlist-overload branch 2 times, most recently from 6204bf9 to 23220da Compare June 25, 2017 14:12
@jayvdb jayvdb changed the title aspectlist: Overload __init__ to accept strings AspectList: Overload __init__ to accept strings Jun 25, 2017
jayvdb
jayvdb requested changes Jun 25, 2017
View reviewed changes
Copy link
Member

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

commit message needs updating to reflect fixed class name.

@adhikasp adhikasp force-pushed the adhikasp/aspectlist-overload branch from 23220da to f7370cc Compare June 25, 2017 14:20
@adhikasp adhikasp force-pushed the adhikasp/aspectlist-overload branch 2 times, most recently from 0d5143b to 928d39f Compare June 25, 2017 16:31
jayvdb
jayvdb requested changes Jun 25, 2017
View reviewed changes
tests/bearlib/aspects/CollectionsTest.py Outdated
'CommitMessage.Body'])
mix_of_aspect = AspectList(['CommitMessage.Shortlog',
Metadata.CommitMessage.Body])
assert isinstance(list_of_aspect, AspectList)
Copy link
Member

Choose a reason for hiding this comment

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

replace assert with self.assert...(...) (This was mentioned to you in another PR just merged; fix all of your PRs)

@adhikasp
AspectList: Overload __init__ to accept strings
852ba5e 
Overload AspectList so it could be instanced from a list of string
containing aspects name. The reason of this overloading is to make
it possible to instanced list of desired aspects name found in
coafiles.

```python
>>> AspectList(['coalaCorrect', 'aspectsYEAH'])
[<aspectclass '...coalaCorrect'>, <aspectclass '...aspectsYEAH'>]
```

Replacement of coala#4379
Closes coala#4382
@adhikasp adhikasp force-pushed the adhikasp/aspectlist-overload branch from 928d39f to 852ba5e Compare June 25, 2017 16:45
@jayvdb jayvdb added process/approved The PR is approved and will be merged soon and removed process/pending review labels Jun 25, 2017
@jayvdb
Copy link
Member

jayvdb commented Jun 25, 2017

ack 852ba5e

@jayvdb
Copy link
Member

jayvdb commented Jun 25, 2017

@Tultor merge

@jayvdb
Copy link
Member

jayvdb commented Jun 25, 2017

@rultor merge

@rultor
Copy link
Contributor

rultor commented Jun 25, 2017

@rultor merge

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

@rultor rultor merged commit 852ba5e into coala:master Jun 25, 2017
@rultor
Copy link
Contributor

rultor commented Jun 25, 2017

@rultor merge

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

@userzimmermann
Copy link
Member

@adhikasp @jayvdb This was merged a bit too early. It misses tests with aspect instances as mentioned in #4389 (comment) . But anyway... We can add that later ;)

@adhikasp adhikasp deleted the adhikasp/aspectlist-overload branch July 7, 2017 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gitmate-bot gitmate-bot gitmate-bot left review comments

@userzimmermann userzimmermann userzimmermann requested changes

@jayvdb jayvdb jayvdb approved these changes

Assignees

@adhikasp adhikasp

Labels
area/aspects process/approved The PR is approved and will be merged soon size/S type/feature
Milestone
#aspectsYEAH
Development

Successfully merging this pull request may close these issues.
5 participants
@adhikasp @jayvdb @rultor @userzimmermann @gitmate-bot