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

Aspect: Create ``map_setting_to_aspect`` decorator #4662

Merged
merged 1 commit into from Aug 26, 2017

Conversation

5 participants
@adhikasp
Copy link
Member

adhikasp commented Aug 24, 2017

@map_setting_to_aspect(
        remove_all_unused_imports=(get_aspect('UnusedImport')
                                   .remove_non_standard_import),
        remove_unused_variables=get_aspect('UnusedLocalVariable'))
def run(self, filename, file,
        remove_all_unused_imports: bool=True,
        remove_unused_variables: bool=True):
    pass

The run argument will be overrided by aspect's setting
(only if it present). This make bear's aspectization much more easy.

Closes #4661

tests/bearlib/aspects/DecoratorsTest.py Outdated
@map_setting_to_aspect(
remove_unreachable_code=get_aspect('UnreachableCode'),
minimum_clone_tokens=(get_aspect('Clone')
.min_clone_tokens),

This comment has been minimized.

@gitmate-bot

gitmate-bot Aug 24, 2017

Collaborator

E128 continuation line under-indented for visual indent

Origin: PycodestyleBear (E128), Section: autopep8.

tests/bearlib/aspects/DecoratorsTest.py Outdated
@map_setting_to_aspect(
remove_unreachable_code=get_aspect('UnreachableCode'),
minimum_clone_tokens=(get_aspect('Clone')
.min_clone_tokens),

This comment has been minimized.

@gitmate-bot

gitmate-bot Aug 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/tests/bearlib/aspects/DecoratorsTest.py
+++ b/tests/bearlib/aspects/DecoratorsTest.py
@@ -18,7 +18,7 @@
     @map_setting_to_aspect(
         remove_unreachable_code=get_aspect('UnreachableCode'),
         minimum_clone_tokens=(get_aspect('Clone')
-                             .min_clone_tokens),
+                              .min_clone_tokens),
     )
     def run(self,
             remove_unreachable_code: bool=False,

@adhikasp adhikasp force-pushed the adhikasp:adhikasp/decor branch Aug 24, 2017

@jayvdb

This comment has been minimized.

Copy link
Member

jayvdb commented Aug 24, 2017

Please dont put code fragments in the commit message.
Just describe the enhancements.
(and your code fragment in the commit body has incorrect indentation anyway.)

coalib/bearlib/aspects/decorators.py Outdated
"""
Override function arguments value with coala's aspect and taste.

This decorators could be used by ``Bear.run()`` to automatically map and

This comment has been minimized.

@hemangsk

hemangsk Aug 24, 2017

Member

decorator* ?

@jayvdb
Copy link
Member

jayvdb left a comment

decorator should use functools.wraps

coalib/bearlib/aspects/__init__.py Outdated
@@ -21,7 +22,7 @@
__all__ = ['Root', 'Taste', 'TasteError',
'aspectclass', 'aspectbase', 'AspectTypeError',
'AspectNotFoundError', 'MultipleAspectFoundError',
'AspectList']
'AspectList', 'map_setting_to_aspect']

This comment has been minimized.

@jayvdb

jayvdb Aug 24, 2017

Member

add trailing comma's and put the final ] on a new line so it doesnt need updating all the time.

@adhikasp adhikasp force-pushed the adhikasp:adhikasp/decor branch Aug 24, 2017

coalib/bearlib/aspects/decorators.py Outdated

def map_setting_to_aspect(**aspectable_setting):
"""
Override function arguments value with coala's aspect and taste.

This comment has been minimized.

@jayvdb

jayvdb Aug 26, 2017

Member

'Override' ?

What happens if a setting is explicitly in .coafile, and is in an aspect? The aspect overrides the explicit setting? I hope not.

This comment has been minimized.

@adhikasp

adhikasp Aug 26, 2017

Author Member

Um yes it is. It will always override explicit or default setting with aspect/taste setting if coala is run under aspect, except for those setting that doesn't mapped here (could be used by a very specific linter setting like pep_ignore) . Do we want different behavior here?

This comment has been minimized.

@adhikasp

adhikasp Aug 26, 2017

Author Member

Priority: old-setting default -> explicit old-setting -> aspect taste default -> explicit aspect taste

This comment has been minimized.

@jayvdb

jayvdb Aug 26, 2017

Member

explicit user choices should never be overriden automatically.
I think it should be:

explicit setting -> explicit aspect taste (if aspects activated in Section) -> aspect taste default (if aspects activated in Section) -> setting default

(please stop calling them 'old-setting' - Setting is still a core component, and people will still keep using them at least until aspects are perfect for everyones needs, which wont be soon, and even then a Setting may still be used internally by the run method, until we also deprecate the run method. )

This comment has been minimized.

@adhikasp

adhikasp Aug 26, 2017

Author Member

Okay I agree, will change it.

This comment has been minimized.

@jayvdb

jayvdb Aug 26, 2017

Member

Then this function docstring needs to change.

coalib/bearlib/aspects/decorators.py Outdated
def _func_decorator(func):
@wraps(func)
def _new_func(self, *args, **kwargs):
if hasattr(self, 'section') and self.section.aspects:

This comment has been minimized.

@jayvdb

jayvdb Aug 26, 2017

Member

Why hasattr(self, 'section') ; i think it should raise an exception if there is no .section?

This comment has been minimized.

@adhikasp

adhikasp Aug 26, 2017

Author Member

Yeah right hasattr(self, 'section') is not necessary here.

@adhikasp adhikasp force-pushed the adhikasp:adhikasp/decor branch Aug 26, 2017

tests/bearlib/aspects/DecoratorsTest.py Outdated
result = self.bear.execute()
self.assertEqual([False, 40], result)


This comment has been minimized.

@gitmate-bot

gitmate-bot Aug 26, 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/DecoratorsTest.py
+++ b/tests/bearlib/aspects/DecoratorsTest.py
@@ -53,7 +53,6 @@
         result = self.bear.execute()
         self.assertEqual([False, 40], result)
 
-
     def test_partial_mapping(self):
         self.section.aspects = AspectList([
             get_aspect('UnreachableCode')('py'),

@adhikasp adhikasp force-pushed the adhikasp:adhikasp/decor branch 2 times, most recently Aug 26, 2017

@adhikasp adhikasp force-pushed the adhikasp:adhikasp/decor branch Aug 26, 2017

Aspect: Create ``map_setting_to_aspect`` decorator
The `run` argument will be overrided by aspect's setting
(only if it present). This make bear's aspectization much more easy.

The priority is:
- explicit setting
- explicit aspect taste (if aspects activated in Section)
- aspect taste default (if aspects activated in Section)
- setting default

Closes #4661

@adhikasp adhikasp force-pushed the adhikasp:adhikasp/decor branch to 76b1e99 Aug 26, 2017

@jayvdb

This comment has been minimized.

Copy link
Member

jayvdb commented Aug 26, 2017

ack 76b1e99

@jayvdb

This comment has been minimized.

Copy link
Member

jayvdb commented Aug 26, 2017

@rultor merge

@jayvdb

jayvdb approved these changes Aug 26, 2017

@rultor

This comment has been minimized.

Copy link
Contributor

rultor commented Aug 26, 2017

@rultor merge

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

@rultor rultor merged commit 76b1e99 into coala:master Aug 26, 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 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.

Copy link
Contributor

rultor commented Aug 26, 2017

@rultor merge

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

@adhikasp adhikasp deleted the adhikasp:adhikasp/decor branch Aug 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.