-
Notifications
You must be signed in to change notification settings - Fork 580
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
aspectsYEAHBear.py: Add aspectsYEAHBear #1604
Conversation
3dd33b1
to
57e086b
Compare
I get 100% coverage when tested locally, don't know what is the problem here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coala/coala#4049 (review)
This is what aspectsYEAHBear
is supposed to do.
Add aspectsYEAHBear to make sure `aspect` is always written with a lower case `a` and `aspectsYEAH` is always written with upper case `YEAH`. Closes coala#1573
57e086b
to
7b886f0
Compare
@pratyushprakash Great work! I will also investagate on your CI issue :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't properly cover both singular 'aspect' and plural 'aspects' cases yet
I also wonder if it's somehow possible to pack all checks into some single regex with matching groups...
To make the CI tests work you first need to require the latest development build of coala
:
https://pypi.python.org/pypi/coala/0.11.0.dev20170411111517
Otherwise there won't be the Spelling.aspectsYEAH
aspect
corrected_aspects = [] | ||
corrected_aspectsYEAH = [] | ||
for line in file: | ||
wrong_spelling1 = r'(?!aspect)[aA][sS][pP][eE][cC][tT]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only check for 'aspect' but not for 'aspects' here
Aspectyeah""" | ||
results = list(self.uut.run('bad_file', bad_file.split('\n'))) | ||
self.assertEqual(results[0].message, | ||
'``aspect`` is always written' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
``aspect`` and ``aspects`` are always...
'[pP][eE][cC][tT][sS][yY][eE][aA][hH]' | ||
if not re.match(wrong_spelling2, line): | ||
corrected_aspects += [re.sub(wrong_spelling1, | ||
'aspects', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here you replace all misspelled 'aspect' with 'aspects'
Just writing this to get a reference to this PR in #1573 for quick access :) |
@pratyushprakash: @jayvdb made a goint point in #1573 (comment) based on my #1573 (comment) We should try to create some shared logic between @coala/aspects-developers |
LICENSE = 'AGPL-3.0' | ||
CAN_DETECT = {'Spelling'} | ||
|
||
def run(self, filename, file, aspects=[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this aspects
in the run function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you wish to run this bear via passing the aspects
or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever it is please explain param aspects
in the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are willing to call it via aspects, the default value has to be Root
perhaps instead of aspectsYEAH
. Well we need to make it universal for all the bears therefore modify it in the run method of Bear class. Remember the discussion we had for the default value. I meant to place it here actually and by mistake I triggered a discussion in the instantiation of Bear class.
LICENSE = 'AGPL-3.0' | ||
CAN_DETECT = {'Spelling'} | ||
|
||
def run(self, filename, file, aspects=[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever it is please explain param aspects
in the docstring.
""" | ||
|
||
for aspect in aspects: | ||
if type(aspect) is aspectsYEAH: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type(aspect) is <aspectclass 'Root.Spelling.aspectsYEAH'>
not aspectsYEAH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better replace with if type(aspect).__name__ is 'aspectsYEAH':
. 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Techievena If aspect
is an instance of aspectsYEAH
then type(aspect)
is the same class object as referenced by aspectsYEAH
. "<aspectclass 'Root.Spelling.aspectsYEAH'>"
is the repr(aspectsYEAH)
. So nothing is wrong with that condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need to implement a more generalized way of running bear logic for supported aspects than in such a for
loop with several if type(aspect) is ...
checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad must have checked properly before reviewing.
@coala/aspects-developers To avoid unnecessary discussions about the value of this bear for now, and to speed up development, we should move it into the coala repo under Then we could also use it as a simple testbed for the whole aspects machinery in coala @pratyushprakash What do you think? Wanna create a new PR with this code in coala? |
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
1 similar comment
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
1 similar comment
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
This is fun and all buuuut maybe we don't want too many fun bears? |
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
Removing from the queue. Re-open if there is consensus on the issue that this is the right way forward. |
Add aspectsYEAHBear to make sure
aspect
is alwayswritten with a lower case
a
andaspectsYEAH
isalways written with upper case
YEAH
.