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

DocBaseClass: Add DocBaseClass #4465

Merged
merged 1 commit into from Jul 22, 2017

Conversation

8 participants
@damngamerz
Member

damngamerz commented Jul 15, 2017

Add DocBaseClass
Merge DocumentationExtraction with DocBaseClass
Closes #2659

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!

tests/bearlib/languages/documentation/DocBaseClassTest.py Outdated
def test_extract_documentation_PYTHON3(self):
def test_DocBaseClass_PYTHON3(self):
Parameter = namedtuple('Parameter', 'name, desc')
ExceptionValue = namedtuple('ExceptionValue', 'name, desc')

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 15, 2017

Collaborator

This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section flakes.

The issue can be fixed by applying the following patch:

--- a/tests/bearlib/languages/documentation/DocBaseClassTest.py
+++ b/tests/bearlib/languages/documentation/DocBaseClassTest.py
@@ -141,7 +141,7 @@
 
     def test_DocBaseClass_PYTHON3(self):
         Parameter = namedtuple('Parameter', 'name, desc')
-        ExceptionValue = namedtuple('ExceptionValue', 'name, desc')
+        namedtuple('ExceptionValue', 'name, desc')
         ReturnValue = namedtuple('ReturnValue', 'desc')
         Description = namedtuple('Description', 'desc')
 
tests/bearlib/languages/documentation/DocBaseClassTest.py Outdated
for doc_comment in BaseDoc.extraction(data, 'PYTHON', 'default'):
parsed = doc_comment.parse()
new_metadata = BaseDoc.fix_parse_data(parsed)

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 15, 2017

Collaborator

This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section flakes.

The issue can be fixed by applying the following patch:

--- a/tests/bearlib/languages/documentation/DocBaseClassTest.py
+++ b/tests/bearlib/languages/documentation/DocBaseClassTest.py
@@ -358,7 +358,7 @@
 
         for doc_comment in BaseDoc.extraction(data, 'PYTHON', 'default'):
             parsed = doc_comment.parse()
-            new_metadata = BaseDoc.fix_parse_data(parsed)
+            BaseDoc.fix_parse_data(parsed)
         expected_warning_desc = """
 Missing function description.
 Please set allow_missing_func_desc = True to ignore this warning.

@damngamerz damngamerz referenced this pull request Jul 15, 2017

Merged

DocumentationStyleBear: Use DocBaseClass #1928

2 of 2 tasks complete
@sils

This comment has been minimized.

Member

sils commented Jul 15, 2017

Builds failing on windows, marking WIP

tests/bearlib/languages/documentation/DocBaseClassTest.py Outdated
Parameter(name='param1',
desc='\n param1 description\n'),
Parameter(name='param2',
desc='\n param2 description\n')])

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 15, 2017

Collaborator

E128 continuation line under-indented for visual indent'

PycodestyleBear (E128), severity NORMAL, section autopep8.

tests/bearlib/languages/documentation/DocBaseClassTest.py Outdated
[Description(
desc='\nMain description comes here.\n\n'),
Parameter(name='param1',
desc='\n param1 description\n'),

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 15, 2017

Collaborator

E128 continuation line under-indented for visual indent'

PycodestyleBear (E128), severity NORMAL, section autopep8.

tests/bearlib/languages/documentation/DocBaseClassTest.py Outdated
[Description(
desc='\nMain description comes here.\n\n'),
Parameter(name='param1',
desc='\n param1 description\n'),

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 15, 2017

Collaborator

The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/tests/bearlib/languages/documentation/DocBaseClassTest.py
+++ b/tests/bearlib/languages/documentation/DocBaseClassTest.py
@@ -428,6 +428,6 @@
                          [Description(
                              desc='\nMain description comes here.\n\n'),
                           Parameter(name='param1',
-                             desc='\n    param1 description\n'),
+                                    desc='\n    param1 description\n'),
                           Parameter(name='param2',
-                             desc='\n    param2 description\n')])
+                                    desc='\n    param2 description\n')])
coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
# annotation directly.
print([stripped_desc[0],3])
if stripped_desc[0] != '':
print([stripped_desc[0],4])

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 15, 2017

Collaborator

E231 missing whitespace after ',''

PycodestyleBear (E231), severity NORMAL, section autopep8.

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
else:
# Wrap parameter description onto next line if it follows
# annotation directly.
print([stripped_desc[0],3])

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 15, 2017

Collaborator

E231 missing whitespace after ',''

PycodestyleBear (E231), severity NORMAL, section autopep8.

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
else:
# Wrap parameter description onto next line if it follows
# annotation directly.
print([stripped_desc[0],3])

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 15, 2017

Collaborator

The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/coalib/bearlib/languages/documentation/DocBaseClass.py
+++ b/coalib/bearlib/languages/documentation/DocBaseClass.py
@@ -345,9 +345,9 @@
             else:
                 # Wrap parameter description onto next line if it follows
                 # annotation directly.
-                print([stripped_desc[0],3])
+                print([stripped_desc[0], 3])
                 if stripped_desc[0] != '':
-                    print([stripped_desc[0],4])
+                    print([stripped_desc[0], 4])
                     stripped_desc.insert(0, '')
 
             # Indent with 4 spaces.
@Nosferatul

This comment has been minimized.

Member

Nosferatul commented Jul 16, 2017

I looked on it. It's really amazing! I don't see how can be improved.

@adhikasp

This comment has been minimized.

Member

adhikasp commented Jul 17, 2017

LGTM

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
docstyle_definition = DocstyleDefinition.load(language, docstyle)
return extract_documentation_with_markers(content, docstyle_definition)
def __init__(self, filename, file):

This comment has been minimized.

@NiklasMM

NiklasMM Jul 18, 2017

Contributor

I don't understand why the Bear is instatiated with file and filename. Aren't those passed to the run method?

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
self.file = file
self.filename = filename
def extraction(self, content, language, docstyle):

This comment has been minimized.

@NiklasMM

NiklasMM Jul 18, 2017

Contributor

This doesn't use any class members and can therefore be a static method.

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
return extract_documentation_with_markers(
content, docstyle_definition)
def fix_parse_data(self, parsed, allow_missing_func_desc: str=False,

This comment has been minimized.

@NiklasMM

NiklasMM Jul 18, 2017

Contributor

Needs docstring.

This comment has been minimized.

@NiklasMM

NiklasMM Jul 18, 2017

Contributor

And the name should probably be fix_parsed_data or something like that.

This comment has been minimized.

@NiklasMM

NiklasMM Jul 18, 2017

Contributor

Also, why is the fixing part of the base class? Should that not be what the concrete Bear does?

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
if main_description.desc == '\n' and not allow_missing_func_desc:
self.warning_desc = """
Missing function description.

This comment has been minimized.

@NiklasMM

NiklasMM Jul 18, 2017

Contributor

I don't like this unindented multiline string. It disrupts the flow when reading the file. Please find another way.

This comment has been minimized.

@NiklasMM

This comment has been minimized.

Contributor

NiklasMM commented Jul 18, 2017

These tests look very untidy now, if you have to use enumeration in your test function names, it's a sign that you could use the pytest.parametrize feature. But please look at my other comments first.

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
def diff_generation(self, doc_comment, new_comment):
"""
Generates diff between the original doc_comment and it's fix

This comment has been minimized.

@NiklasMM

NiklasMM Jul 20, 2017

Contributor

it's -> its

This comment has been minimized.

@damngamerz

damngamerz Jul 20, 2017

Member

done as well 👍

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
diff.replace(old_range, new_comment.assemble())
self.affected_code = (diff.range(self.filename),)
self.diffs = {self.filename: diff}

This comment has been minimized.

@NiklasMM

NiklasMM Jul 20, 2017

Contributor

Why are diffs not just returned by this method? What's the benefit of storing them in member in this fashion?

This comment has been minimized.

@damngamerz

damngamerz Jul 20, 2017

Member

we need self.diffs, self.affected_code at the bear side where RESULT is yielded.

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
self.file = None
self.filename = None
def extraction(self, language, docstyle):

This comment has been minimized.

@NiklasMM

NiklasMM Jul 20, 2017

Contributor

I'd prefer to name the methods in a verbal form, so extract in this case

This comment has been minimized.

@damngamerz

damngamerz Jul 20, 2017

Member

done 👍

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
return extract_documentation_with_markers(
self.file, docstyle_definition)
def diff_generation(self, doc_comment, new_comment):

This comment has been minimized.

@NiklasMM

NiklasMM Jul 20, 2017

Contributor

Same here, generate_diff

This comment has been minimized.

@damngamerz

damngamerz Jul 20, 2017

Member

done 👍

tests/bearlib/languages/documentation/DocBaseClassTest.py Outdated
from coalib.results.TextPosition import TextPosition
class DocumentationExtractionTest(unittest.TestCase):
BaseDoc = DocBaseClass()

This comment has been minimized.

@NiklasMM

NiklasMM Jul 20, 2017

Contributor

Having a module scoped variable that is used in all tests is not a good idea. You should do that in a pytest fixture.
Also the name is kind of nonsensical, BaseDoc? Is it a base document?

This comment has been minimized.

@damngamerz

damngamerz Jul 20, 2017

Member

I need an instance of DocBaseClass to test it, just like in case of LocalBear. hmm need to think about different name though.

@gitmate-bot gitmate-bot added size/S and removed size/M labels Jul 20, 2017

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
:param doc_comment:
original instance of DocumentationComment
:param new_comment:
fix instance of DocumentationComment

This comment has been minimized.

@NiklasMM

NiklasMM Jul 21, 2017

Contributor

*fixed

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
:return: An iterator returning each DocumentationComment
found in the content.
DocBaseClass holds important function which will extract, parse
and generates diffs for documentation.

This comment has been minimized.

@NiklasMM

NiklasMM Jul 21, 2017

Contributor

You can expand a bit on this docstring, for example add "All bears that process documentation should inherit from this" or something.

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
:param new_comment:
fix instance of DocumentationComment
:return:
a tuple of affected_code and diffs

This comment has been minimized.

@NiklasMM

NiklasMM Jul 21, 2017

Contributor

Why diffs? it's only one isn't it?

tests/bearlib/languages/documentation/DocBaseClassTest.py Outdated
data_new, 'PYTHON3', 'default'):
new_doc_comment = doc_comment
(affected_code, diffs) = DocBaseClass.generate_diff(

This comment has been minimized.

@NiklasMM

NiklasMM Jul 21, 2017

Contributor

Add tests on the generated diff, too. Not just the affected_code object.

This comment has been minimized.

@damngamerz

damngamerz Jul 21, 2017

Member

We dont need this now with the new changes I guess. I will directly compare the diff object.

tests/bearlib/languages/documentation/DocBaseClassTest.py Outdated
[])
def test_diff_generation(self):

This comment has been minimized.

@NiklasMM

NiklasMM Jul 21, 2017

Contributor

What happens if the two DocumentationComment objects are the same? You should add a test for that.

This comment has been minimized.

@damngamerz

damngamerz Jul 21, 2017

Member

Check the bear part... we only use generate_diff if doc_comment != new_comment .still we need a case for that ?

coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
diff.replace(old_range, new_comment.assemble())
affected_code = (diff.range(filename),)
diffs = {filename: diff}

This comment has been minimized.

@NiklasMM

NiklasMM Jul 21, 2017

Contributor

Oh, I see. This is the kind of object accepted by the Result constructor. Please don't construct this here, but rather in the run method of the bear to reduce tight coupling.

This comment has been minimized.

@damngamerz

damngamerz Jul 21, 2017

Member

Yup I see I should return the diff object 👍 Even This would reduce 2 arguments passed 👍

tests/bearlib/languages/documentation/DocBaseClassTest.py Outdated
from tests.bearlib.languages.documentation.TestUtils import (
load_testdata)
from coalib.results.TextPosition import TextPosition
from coalib.results.TextRange import TextRange
from coalib.results.Diff import Diff
from coalib.settings.Section import Section

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 21, 2017

Collaborator

This file contains unused source code.

Origin: PyUnusedCodeBear, Section: flakes.

The issue can be fixed by applying the following patch:

--- a/tests/bearlib/languages/documentation/DocBaseClassTest.py
+++ b/tests/bearlib/languages/documentation/DocBaseClassTest.py
@@ -11,7 +11,6 @@
 from coalib.results.TextPosition import TextPosition
 from coalib.results.TextRange import TextRange
 from coalib.results.Diff import Diff
-from coalib.settings.Section import Section
 
 
 class DocBaseClassTest(unittest.TestCase):
coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
from coalib.results.TextPosition import TextPosition
from coalib.bears.LocalBear import LocalBear

This comment has been minimized.

@gitmate-bot

gitmate-bot Jul 21, 2017

Collaborator

This file contains unused source code.

Origin: PyUnusedCodeBear, Section: flakes.

The issue can be fixed by applying the following patch:

--- a/coalib/bearlib/languages/documentation/DocBaseClass.py
+++ b/coalib/bearlib/languages/documentation/DocBaseClass.py
@@ -7,8 +7,6 @@
 from coalib.results.Diff import Diff
 from coalib.results.TextRange import TextRange
 from coalib.results.TextPosition import TextPosition
-from coalib.bears.LocalBear import LocalBear
-from coalib.results.Result import Result
 
 
 def _extract_doc_comment_simple(content, line, column, markers):
coalib/bearlib/languages/documentation/DocBaseClass.py Outdated
Checks and handles the fixing part of documentation.
:return:
a tuple of processed documentation and warning_desc

This comment has been minimized.

@SanketDG

SanketDG Jul 21, 2017

Member

A capital

This comment has been minimized.

@damngamerz

damngamerz Jul 21, 2017

Member

Just following the PEP-257 convention. (Even java follows the same )
comments desc - all lower and doesn't ends with a period. 😄

This comment has been minimized.

@SanketDG

SanketDG Jul 22, 2017

Member

I do not see the above mentioned anywhere in pep 257

DocBaseClass: Add DocBaseClass
Add DocBaseClass
Merge DocumentationExtraction with DocBaseClass
Closes #2659
@SanketDG

This comment has been minimized.

Member

SanketDG commented Jul 22, 2017

ack d79e497

@SanketDG

This comment has been minimized.

Member

SanketDG commented Jul 22, 2017

@rultor merge

@rultor

This comment has been minimized.

Contributor

rultor commented Jul 22, 2017

@rultor merge

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

@rultor rultor merged commit d79e497 into coala:master Jul 22, 2017

6 of 8 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
codecov/project 100% (target 100%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
review/gitmate/commit This commit has no issues. :)
Details
review/gitmate/manual This commit was acknowledged.
Details
review/gitmate/pr This PR has no issues. :)
Details

@damngamerz damngamerz deleted the damngamerz:docbaseclass branch Jul 22, 2017

@rultor

This comment has been minimized.

Contributor

rultor commented Jul 22, 2017

@rultor merge

@SanketDG 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