cEP-0005: Improve bear and aspect design #28
Conversation
|
Thanks for your contribution! Reviewing pull requests take really a lot of time and we're all volunteers. Please make sure you go through the following check list and complete them all before pinging someone for a review.
As you learn things over your Pull Request please help others on the chat and on PRs to get their stuff right as well! |
| """ | ||
|
|
||
| # This is a leaf of the tree! It has to be well documented and bears will only | ||
| # implement leafs. However users may use just Metadata.Shortlog to check for |
| class CommitMessage: | ||
| """ | ||
| Your commit message is important documentation associated with your | ||
| source code. It can help you to identify bugs (e.g. through |
There was a problem hiding this comment.
to identify or identifying, I prefer the first in this case
| """ | ||
|
|
||
| # This is a leaf of the tree! It has to be well documented and bears will only | ||
| # implement leafs. However users may use just Metadata.Shortlog to check for |
| # implement leafs. However users may use just Metadata.Shortlog to check for | ||
| # groups of aspects. This simplifies browsing aspect documentation: | ||
| # | ||
| # Wether the user wants to see/configure aspects precisely or roughly, he can |
| @Shortlog.subaspect | ||
| class ColonExistence: | ||
| """ | ||
| Some projects force to use colons in the commit message shortlog (first |
| @@ -102,18 +102,25 @@ class RedundancyBear(LocalBear): | |||
| # This bear is supposed to be able to fix unused imports... | |||
| FIX_ASPECTS = {Root.Redundancy.UnusedImport} | |||
| # ... and detect code clones. | |||
| DETECT_ASPECTS = {Root.Redundancy.CloneRedundancy} | |||
| DETECT_ASPECTS = {Root.Redundancy.Clone} | |||
|
|
|||
| # Aspects are passed as parameter | |||
| def run(self, filename, file, aspects): | |||
There was a problem hiding this comment.
Would be easier to use a generalized run() from Bear base class which gets **aspect_settings and then just iterates over FIX_ASPECTS and DETECT_ASPECTS (and WHATEVER_ASPECTS), instantiates each selected one with the aspect_settings, and runs registered bear-specific checker functions for every aspect, as commented below the run() method. This would also avoid any .active checks and running code and generating results for unselected aspects.
There was a problem hiding this comment.
see below, I like the idea, that would be an own cEP IMO and can be implemented orthogonally to this, so we can do it later.
|
|
||
| # The aspect is tied to the result. coala now knows everything from | ||
| # the aspect documentation! | ||
| yield Result.from_values(aspect=Root.Redundancy.Clone, ...) |
There was a problem hiding this comment.
Bear base class could provide some .aspect_checker decorator:
@classmethod
def aspect_checker(cls, aspectcls):
def checker_deco(func):
cls.ASPECT_CHECKERS[aspectcls] = func
return func
return checker_decoWhich is then used like:
@RedundancyBear.aspect_checker(Root.Redundancy.Clone)
def clone_checker(bear, filename, file, aspect):
min_clone_tokens = aspect.min_clone_tokens
return ResultValues(name='value', ...) # Result.from_values args except aspect=And a generalized run() method could then work like:
def run(self, *args, **aspect_settings):
for aspectcls in self.FIX_ASPECTS.union(self.DETECT_ASPECTS):
if not selected(aspectcls):
continue
checker_func = self.ASPECT_CHECKERS.get(aspectcls)
if checker_func is None:
continue
aspect = aspectcls(**aspect_settings)
result_values = checker_func(self, *args, aspect=aspect)
yield Result.from_values(aspect=aspectcls, **result_values)There was a problem hiding this comment.
@...aspect_checker functions could of course also yield several ResultValues()
There was a problem hiding this comment.
Still some problems exist with that approach, like when several aspects depend on a common logic, which should only be processed once. Or a missing proper run() argspec in derived bears.
There was a problem hiding this comment.
if checker_func is None: in run() above should of course raise NotImplementedError instead of just continue
There was a problem hiding this comment.
I think in the usual case this won't be possible easily because of shared logic, we might however include that possibility in the bear base class at a later point. This is however nothing that changes the main design so I wouldn't consider it yet especially as it probably comes with a performance penalty at least in most possible usecases (due to shared code)
|
Forgot to submit my review from Friday :D |
| if unused_imports(file): | ||
| # A bear may yield results for any aspect even if it's not selected. | ||
| # coala will filter out only selected aspects. | ||
| yield Result.from_values(aspect=Root.Redundancy.UnusedImprt, ...) |
| # Check the code ... | ||
| if unused_imports(file): | ||
| # A bear may yield results for any aspect even if it's not selected. | ||
| # coala will filter out only selected aspects. |
There was a problem hiding this comment.
We should trigger a warning in this case^^
There was a problem hiding this comment.
nope, the bear may or may not use the aspects to perf optimize, it may just yield everything. Convenience feature esp. useful for linter bears
|
|
||
| # Check the code ... | ||
| if unused_imports(file): |
There was a problem hiding this comment.
not valid code, missing pass or some other meaningful code.
There was a problem hiding this comment.
it's valid, there's a yield coming
|
ack 07696f7 |
|
reack ed58ca3 |
|
ack ed58ca3 |
|
@rultor merge |
@userzimmermann OK, I'll try to merge now. You can check the progress of the merge here |
@userzimmermann Done! FYI, the full log is here (took me 1min) |
|
Not an objection, (and I havent even started looking at the details of aspects) ... just a comment in case it is relevant ...
I believe there are going to be many cases where a linter bear needs to listen to an aspect, in order to reject configuration problems. e.g. coala/coala-bears#897 |
CC @userzimmermann for review