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 DocGrammarBear for docstrings #1980

Merged
merged 1 commit into from Aug 16, 2017
Merged

Conversation

damngamerz
Copy link
Member

@damngamerz damngamerz commented Aug 5, 2017

This bear checks for spellings and grammatical mistakes
on the descriptions of documentation comments.

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

  • I read the commit guidelines and I've followed
    them.
  • I ran coala over my code locally. (All commits have to pass
    individually.
    It is not sufficient to have "fixup commits" on your PR,
    our bot will still report the issues for the previous commit.) You will
    likely receive a lot of bot comments and build failures if coala does not
    pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

test_bad1 = test('bad_file.py.test', 'bad_file.py.test.correct')
test_good1 = test('good_file.py.test', 'good_file.py.test')
test_good2 = test('good_file2.py.test', 'good_file2.py.test', {
'languagetool_disable_rules': 'UPPERCASE_SENTENCE_START'})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is longer than allowed. (80 > 79)

LineLengthBear, severity NORMAL, section linelength.

test_bad1 = test('bad_file.py.test', 'bad_file.py.test.correct')
test_good1 = test('good_file.py.test', 'good_file.py.test')
test_good2 = test('good_file2.py.test', 'good_file2.py.test', {
'languagetool_disable_rules': 'UPPERCASE_SENTENCE_START'})
Copy link
Collaborator

Choose a reason for hiding this comment

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

E501 line too long (80 > 79 characters)'

PycodestyleBear (E501), severity NORMAL, section autopep8.

@gitmate-bot
Copy link
Collaborator

Comment on ae7a1d9.

No newline found between shortlog and body at HEAD commit. Please add one.

GitCommitBear, severity NORMAL, section commit.

@damngamerz damngamerz force-pushed the docgrammarbear branch 2 times, most recently from b59185e to 05f9d01 Compare August 5, 2017 20:37
Copy link
Contributor

@NiklasMM NiklasMM left a comment

Choose a reason for hiding this comment

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

Please do not read test data from files, it's very hard to read the tests this way. Rather define the file content inside the test file. You can do that using string literals or even more conveniently, by building DocComment objects and assembling them

natural_language,
languagetool_disable_rules):
"""
This fixes the parsed documentation comment.
Copy link
Contributor

Choose a reason for hiding this comment

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

How?

Copy link
Member Author

Choose a reason for hiding this comment

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

grammatically/spell correcting. need to add this 👍

Copy link
Member

Choose a reason for hiding this comment

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

also please mention the tool, and its links


:param parsed:
Contains parsed documentation comment.
:param natural_language:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not call the parameter locale?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we need to specify language with locale en-US. These settings similar to the ones in LanguageToolBear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, yeah I see that it's take from there. But I'd still the parameter locale, that way users immediately know what input we expect.

Copy link
Member Author

Choose a reason for hiding this comment

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

cool then lets have locale 👍

languagetool_disable_rules: typed_list(str)=()):
"""
Checks the main description and comments description of documentation
with LanguageTool.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like a little summary of what language tool can fix plus a link to the documentation.

AUTHORS_EMAILS = {'coala-devel@googlegroups.com'}
LICENSE = 'AGPL-3.0'
CAN_DETECT = {'Documentation', 'Spelling', 'Grammar'}
CAN_FIX = {'Documentation', 'Spelling', 'Grammar'}
Copy link
Member

Choose a reason for hiding this comment

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

can we have a cool asciinema?

natural_language,
languagetool_disable_rules):
"""
This fixes the parsed documentation comment.
Copy link
Member

Choose a reason for hiding this comment

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

also please mention the tool, and its links

test_good1 = test('good_file.py.test', 'good_file.py.test')
test_good2 = test(
'good_file2.py.test', 'good_file2.py.test',
{'languagetool_disable_rules': 'UPPERCASE_SENTENCE_START'})
Copy link
Member

Choose a reason for hiding this comment

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

Add example: Fix spelling


try:
import language_check
language_check
Copy link
Member

Choose a reason for hiding this comment

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

this line needed?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

this line needed?

Yes as the same is followed in LanguageToolBear.

@@ -0,0 +1,108 @@
from queue import Queue
import os.path
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section flakes.

The issue can be fixed by applying the following patch:

--- a/tests/documentation/DocGrammarBearTest.py
+++ b/tests/documentation/DocGrammarBearTest.py
@@ -1,7 +1,5 @@
 from queue import Queue
-import os.path
 import unittest
-import shutil
 
 from coalib.results.Diff import Diff
 from coalib.settings.Section import Section

@damngamerz damngamerz force-pushed the docgrammarbear branch 2 times, most recently from a74c55b to f42423d Compare August 9, 2017 15:06
LanguageTool
correct
return True
except ImportError: # pragma: no cover
Copy link
Collaborator

Choose a reason for hiding this comment

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

E261 at least two spaces before inline comment'

PycodestyleBear (E261), severity NORMAL, section autopep8.

LanguageTool
correct
return True
except ImportError: # pragma: no cover
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/bears/documentation/DocGrammarBear.py
+++ b/bears/documentation/DocGrammarBear.py
@@ -33,7 +33,7 @@
                 LanguageTool
                 correct
                 return True
-            except ImportError: # pragma: no cover
+            except ImportError:  # pragma: no cover
                 return 'Please install the `language-check` pip package.'
 
     def process_documentation(self,

@damngamerz damngamerz force-pushed the docgrammarbear branch 2 times, most recently from 861c8fb to 4983264 Compare August 9, 2017 15:12
AUTHORS_EMAILS = {'coala-devel@googlegroups.com'}
LICENSE = 'AGPL-3.0'
ASCIINEMA_URL = 'https://asciinema.org/a/132564'
CAN_DETECT = {'Documentation', 'Spelling', 'Grammar'}
Copy link
Member

Choose a reason for hiding this comment

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

We don't need a CAN_DETECT if we're already mentioning a CAN_FIX 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

is it? I see....

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, because if we can fix something, we have already detected that thing. 😄

from coalib.testing.LocalBearTestHelper import execute_bear
from coalib.testing.BearTestHelper import generate_skip_decorator
from bears.documentation.DocGrammarBear import DocGrammarBear
from string import Template
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should leave a line and then do the DocGrammarBear import.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup looks good that way...
also Template import should be at top I feel.

Copy link
Member

Choose a reason for hiding this comment

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

We have an unofficial imports style (absent in most bears) in which we do the bear imports at the bottom. You can have a glance over the PEP8 import style. 😉

Copy link
Member

Choose a reason for hiding this comment

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

yeah move it to the top

Copy link
Member

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

looks good, small changes needed

from coalib.testing.LocalBearTestHelper import execute_bear
from coalib.testing.BearTestHelper import generate_skip_decorator
from bears.documentation.DocGrammarBear import DocGrammarBear
from string import Template
Copy link
Member

Choose a reason for hiding this comment

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

yeah move it to the top

import language_check
language_check
except ImportError as err:
raise SkipTest(str(err))
Copy link
Member

Choose a reason for hiding this comment

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

I gave review comments on this on using a decorator

Copy link
Member

Choose a reason for hiding this comment

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

Oh you used it, then no need for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is never importet and breaks my test suite :P

'\n' +
':param xyz: $param_description' +
':return: $return_description' +
'"""\n')
Copy link
Member

Choose a reason for hiding this comment

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

Now I am not so sure about using Template, .format() does this job easly

Copy link
Member

Choose a reason for hiding this comment

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

yeah let's just go with format https://pyformat.info/

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, also please use a multi line string with dedent. It's much easiert to read.

raise SkipTest(str(err))


def make_docstring(main_desc: str='',
Copy link
Member

Choose a reason for hiding this comment

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

we need to explain this function, add some docs

Copy link
Contributor

@NiklasMM NiklasMM left a comment

Choose a reason for hiding this comment

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

Looks good, the test suite looks much nicer now. I still recommend you use pytest's parametrize to get rid of more boilerplate code.

Things I'm missing in the testsuite:

  • Tests for a programming language that isn't Python
  • Tests for a natural language that isn't English

and grammatic rules via LanguageTool.

:param parsed:
Contains parsed documentation comment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a DocComment object? If so, why not mention it?

Copy link
Member Author

Choose a reason for hiding this comment

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

no this is DocComment.parse() parsed documentation comment.

'\n' +
':param xyz: $param_description' +
':return: $return_description' +
'"""\n')
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, also please use a multi line string with dedent. It's much easiert to read.

return_desc='Nothing.\n'),
{'languagetool_disable_rules': 'UPPERCASE_SENTENCE_START'})

test_explicit = test([
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment explainting what the test does.

).splitlines(True)


def test(test_file, expected_file, optional_setting=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be the core function of your test suite, why not give it a docstring?

Copy link
Member Author

Choose a reason for hiding this comment

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

we are giving a docstring/file data(explicit tests).
Maybe I should rename them to test_data, expected_data

make_docstring(main_desc='This sentence doesn\'t have an '
'apostrophe\n'))

test_correct_grammar = test(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of making the same docstring twice, make it once and pass it to the function twice. This make it immediately clear, that nothing changes.

import language_check
language_check
except ImportError as err:
raise SkipTest(str(err))
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is never importet and breaks my test suite :P

make_docstring(main_desc='Il monte s’il veut.\n'),
{'locale': 'fr',
'languagetool_disable_rules': 'FRENCH_WHITESPACE'})

Copy link
Member Author

Choose a reason for hiding this comment

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

@damngamerz damngamerz force-pushed the docgrammarbear branch 2 times, most recently from a4a1066 to 94f6f1b Compare August 12, 2017 19:09
# which was breaking this test case.
test_language_french = unittest.skipIf(
platform.system() == 'Windows',
"language-check fails this test on windows")(
Copy link
Collaborator

Choose a reason for hiding this comment

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

You do not use the preferred quotation marks.

QuotesBear, severity NORMAL, section python.

The issue can be fixed by applying the following patch:

--- a/tests/documentation/DocGrammarBearTest.py
+++ b/tests/documentation/DocGrammarBearTest.py
@@ -119,7 +119,7 @@
     # which was breaking this test case.
     test_language_french = unittest.skipIf(
         platform.system() == 'Windows',
-        "language-check fails this test on windows")(
+        'language-check fails this test on windows')(
             test(
                 make_docstring(main_desc='il monte en haut si il veut.\n'),
                 make_docstring(main_desc='Il monte s’il veut.\n'),

@damngamerz damngamerz force-pushed the docgrammarbear branch 2 times, most recently from 51c80c1 to 3918169 Compare August 12, 2017 19:27
@damngamerz damngamerz force-pushed the docgrammarbear branch 2 times, most recently from 369d4f8 to b86b53b Compare August 15, 2017 16:38
This bear checks for spellings and grammatical mistakes
on the descriptions of documentation comments.
@SanketDG
Copy link
Member

ack 112fa7d

@SanketDG
Copy link
Member

@rultor merge

@rultor
Copy link

rultor commented Aug 16, 2017

@rultor merge

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

@rultor rultor merged commit 112fa7d into coala:master Aug 16, 2017
@rultor
Copy link

rultor commented Aug 16, 2017

@rultor merge

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

@damngamerz damngamerz deleted the docgrammarbear branch August 16, 2017 14:00
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

6 participants