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

aspectsYEAHBear.py: Add aspectsYEAHBear #1604

Closed
wants to merge 1 commit into from
Closed

aspectsYEAHBear.py: Add aspectsYEAHBear #1604

wants to merge 1 commit into from

Conversation

pratyushprakash
Copy link
Contributor

@pratyushprakash pratyushprakash commented Apr 11, 2017 •
edited
Loading

Add aspectsYEAHBear to make sure aspect is always
written with a lower case a and aspectsYEAH is
always written with upper case YEAH.

@pratyushprakash pratyushprakash changed the title aspectsYEAHBear.py: Add aspectYEAHBear aspectsYEAHBear.py: Add aspectsYEAHBear Apr 11, 2017
@pratyushprakash
Copy link
Contributor Author

I get 100% coverage when tested locally, don't know what is the problem here.

Techievena
Techievena suggested changes Apr 11, 2017
View reviewed changes
Copy link
Member

@Techievena Techievena left a 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.

@jayvdb jayvdb mentioned this pull request Apr 11, 2017
Open
@pratyushprakash
aspectsYEAHBear.py: Add aspectsYEAHBear
7b886f0 
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
@userzimmermann
Copy link
Member

@pratyushprakash Great work! I will also investagate on your CI issue :)

userzimmermann
userzimmermann requested changes Apr 11, 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.

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

bears/general/aspectsYEAHBear.py
corrected_aspects = []
corrected_aspectsYEAH = []
for line in file:
wrong_spelling1 = r'(?!aspect)[aA][sS][pP][eE][cC][tT]'
Copy link
Member

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

tests/general/aspectsYEAHBearTest.py
Aspectyeah"""
results = list(self.uut.run('bad_file', bad_file.split('\n')))
self.assertEqual(results[0].message,
'``aspect`` is always written'
Copy link
Member

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...

bears/general/aspectsYEAHBear.py
'[pP][eE][cC][tT][sS][yY][eE][aA][hH]'
if not re.match(wrong_spelling2, line):
corrected_aspects += [re.sub(wrong_spelling1,
'aspects',
Copy link
Member

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'

@userzimmermann
Copy link
Member

Just writing this to get a reference to this PR in #1573 for quick access :)

@userzimmermann userzimmermann mentioned this pull request Apr 11, 2017
Open
@userzimmermann
Copy link
Member

userzimmermann commented Apr 11, 2017 •
edited
Loading

@pratyushprakash: @jayvdb made a goint point in #1573 (comment) based on my #1573 (comment)

We should try to create some shared logic between coalaBear and aspectsYEAHBear as part of this PR :) I definitely support this idea from a purely technical view! And furthermore I'm sure we can then also get back to aspects harmony and remove the status/blocked label ;)

@coala/aspects-developers

Techievena
Techievena reviewed Apr 12, 2017
View reviewed changes
bears/general/aspectsYEAHBear.py
LICENSE = 'AGPL-3.0'
CAN_DETECT = {'Spelling'}

def run(self, filename, file, aspects=[
Copy link
Member

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?

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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.

Techievena
Techievena suggested changes Apr 12, 2017
View reviewed changes
bears/general/aspectsYEAHBear.py
LICENSE = 'AGPL-3.0'
CAN_DETECT = {'Spelling'}

def run(self, filename, file, aspects=[
Copy link
Member

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.

bears/general/aspectsYEAHBear.py
"""

for aspect in aspects:
if type(aspect) is aspectsYEAH:
Copy link
Member

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

Copy link
Member

@Techievena Techievena Apr 12, 2017 •
edited
Loading

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':. 😃

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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.

@userzimmermann
Copy link
Member

@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 coalib.bearlib.aspects

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?

@gitmate-bot
Copy link
Collaborator

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
@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@userzimmermann userzimmermann mentioned this pull request Apr 27, 2017
Open
@gitmate-bot
Copy link
Collaborator

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
@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@sims1253
Copy link
Member

sims1253 commented May 5, 2017

This is fun and all buuuut maybe we don't want too many fun bears?
How about we get a "not so serious bear repository" so we keep the useful ones and the fun project ones separate?

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@jayvdb
Copy link
Member

jayvdb commented May 18, 2017

Removing from the queue. Re-open if there is consensus on the issue that this is the right way forward.

@jayvdb jayvdb closed this May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@userzimmermann userzimmermann userzimmermann requested changes

@Techievena Techievena Techievena requested changes

Assignees

@pratyushprakash pratyushprakash

Labels
area/aspects importance/low process/pending review size/L status/blocked
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pratyushprakash @userzimmermann @gitmate-bot @sims1253 @jayvdb @Techievena @coala-bot