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

Add docs on writing aspect bear #4666

Merged
merged 1 commit into from Aug 29, 2017
Merged

Conversation

adhikasp
Copy link
Member

Relates to #4665
Contain commit from #4662

https://github.com/coala/cEPs/blob/master/cEP-0005.md.

An aspect-compliant bear MUST:
1. Declare list of aspect it can fix and detected. Note that the aspect MUST be
Copy link
Member Author

Choose a reason for hiding this comment

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

Blank line here

3. Map setting to its equivalent aspect or taste using ``map_setting_to_aspect``
decorator.
4. Anotate yielded result with ``aspect``.

Copy link
Member

Choose a reason for hiding this comment

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

Annotate*

"""
Override function arguments value with coala's aspect and taste.

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

Choose a reason for hiding this comment

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

Suggestion: decorator can* might be better

@Nosferatul
Copy link
Member

it looks good to me 👍

@jayvdb jayvdb added status/blocked The issue requires other referenced issues/PRs to be solved/merged before being worked on and removed process/pending review labels Aug 26, 2017
@jayvdb
Copy link
Member

jayvdb commented Aug 26, 2017

This is waiting on #4662

@jayvdb jayvdb removed the status/blocked The issue requires other referenced issues/PRs to be solved/merged before being worked on label Aug 27, 2017
def run(self,
filename,
file,
use_standard_dictionary:bool=True,
Copy link
Member

Choose a reason for hiding this comment

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

We typically have a space after :, which is the general style at https://www.python.org/dev/peps/pep-0484/

languages=['Python']):

@map_setting_to_aspect(
use_standard_dictionary=DictionarySpelling
Copy link
Member

Choose a reason for hiding this comment

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

comma not needed here??
please run your sample code through linters.

class SpellingCheckBear(
LocalBear,
aspect={
'detect': [DictionarySpelling, OrgSpecificWordSpelling]
Copy link
Member

Choose a reason for hiding this comment

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

trailing comma

if word not in dictionary_words:
yield self.new_result(
message='Wrong spelling',
additional_info=(DictionarySpelling.docs
Copy link
Member

Choose a reason for hiding this comment

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

shouldnt DictionarySpelling.docs.importance_reason be the default value for additional_info?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it should be. If you mean

If result have aspect arg, it should be defaulting to use aspect.docs.importance_reason

then, no, it is not implemented yet.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a bug about it?! If not, please create one!
This would fall in your main project scope, of making it easy to use aspects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #4680

Copy link
Member Author

Choose a reason for hiding this comment

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

That issue is included in this PR too.

for word in file.split():
if word not in dictionary_words:
yield self.new_result(
message='Wrong spelling',
Copy link
Member

Choose a reason for hiding this comment

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

this message should include the value of word

# Imagine this is where we save our standard dictionary.
dictionary_words = ['lorem', 'ipsum']

if not use_standard_dictionary:
Copy link
Member

Choose a reason for hiding this comment

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

this logic is tortured.
use

if ...:
    dictionary_words = ['lorem', 'ipsum']
else:
    dictionary_words = []

@adhikasp
Copy link
Member Author

Contain additional commit to fix #4682

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.

not tested properly.

jayvdb
jayvdb approved these changes Aug 28, 2017
@jayvdb
Copy link
Member

jayvdb commented Aug 28, 2017

ack d41d691

@Nosferatul
Copy link
Member

rebase and LGTM

jayvdb
jayvdb approved these changes Aug 29, 2017
@jayvdb
Copy link
Member

jayvdb commented Aug 29, 2017

reack 8c9141c

@jayvdb
Copy link
Member

jayvdb commented Aug 29, 2017

@rultor merge

@rultor
Copy link
Contributor

rultor commented Aug 29, 2017

@rultor merge

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

@rultor rultor merged commit 8c9141c into coala:master Aug 29, 2017
@rultor
Copy link
Contributor

rultor commented Aug 29, 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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants