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

DocumentationAPI: Add padding #4557

Merged
merged 1 commit into from
Aug 18, 2017
Merged

DocumentationAPI: Add padding #4557

merged 1 commit into from
Aug 18, 2017

Conversation

damngamerz
Copy link
Member

@damngamerz damngamerz commented Jul 25, 2017

Add padding in DocstyleDefinition which
are now acquired from .coalang. Add
top_padding and bottom_padding which
automatically instantiate from
instantiate_padding in DocBaseClass. Add
supporting test cases.

Related to #4200

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!

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.

I see you have not add padding for all other languages.
But you make padding mandatory.
So this would break all languages without a padding in their style?

@@ -58,6 +60,8 @@ def __init__(self, language: str, docstyle: str, markers: (Iterable, str),
'actually {}).'.format(length))

self._metadata = metadata
self._top_padding = None if padding == () else int(padding[0])
self._bottom_padding = None if padding == () else int(padding[1])

Copy link
Member Author

Choose a reason for hiding this comment

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

@jayvdb This answers your question :) . .coalang will have official standards defined.
By default top_padding and bottom_padding of DocComment are initialized with zero. If we don't have style defined. It will have default values. Also padding's major motive was if a developer doesn't wants to follow official standard and decides suppose I want to have 2 blank lines at the bottom of docstring. He will set those padding variables and it will be amended by the bear part.

Copy link
Member

Choose a reason for hiding this comment

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

Why split this into to variables rather than store a tuple?

Copy link
Member Author

Choose a reason for hiding this comment

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

using separate variables it becomes easily available at bear side. DocComment.docstyle_definition.top_padding/bottom_padding Also they are read as strings from the file(need int). I need to get separate values in the bear part anyhow. So why not split them in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

You can used NamedTuple to make it easier for them to be used.
The reason to keep them as a tuple is so that everywhere they are a pair, from style config to bear. consistency.

@@ -21,7 +21,7 @@ class DocstyleDefinition:

@enforce_signature
def __init__(self, language: str, docstyle: str, markers: (Iterable, str),
metadata: Metadata):
metadata: Metadata, padding: (Iterable, str)):
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to provide a default value for this new arg, thereby avoiding breaking the API?

Copy link
Member Author

@damngamerz damngamerz Jul 26, 2017

Choose a reason for hiding this comment

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

let me see though. But I don't think that would cause any problem because load function provides an empty tuple if it finds nothing. Which I later break into the properties of top_padding and bottom_padding. which is taken care by #4557 (comment)

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 this breaks the API. I tried to provide a default value. But the thing is load returns an empty tuple if it finds nothing which overwrites that default value. The best solution to providing default values through https://github.com/coala/coala/pull/4557/files#diff-bb503d77ea6d0498c9865cab08c904f4R64

# following documentation.
if ((doc_comment.marker[2]+'\n') != content[end_index-1][-4:]
and bottom_padding == 0):
break
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 is important I couldn't find any other way to do this :)

@damngamerz damngamerz force-pushed the blankline branch 3 times, most recently from 54b9fac to 84b0386 Compare July 26, 2017 19:33
@damngamerz
Copy link
Member Author

damngamerz commented Jul 26, 2017

@jayvdb We can continue our discussion here.... As default.coalang will take care of the intended blank lines.
So for python I believe is that PEP-257 is old. Even we don't follow it's official multi line docstring. And I feel what we use here at coala is much better(which is an enhanced inherited version of PEP - 257 standards).
atm we don't have any mechanism to describe the docstring type. Is it normal function docstring or class function docstring?
so any ideas what can be top_padding and bottom_padding which can be applied to all docstrings in coala repo, coala-bears? my bets are on (0,1). As generally we need to have a blank line after the docstring(such as in class function).
Or if we don't implement this now, leave it as (0,0) cause this is a trivial task. And find some way to get docstring origin(function docstring, class function docstring....etc).

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.

All of the examples seem to be using padding (1, 1).

I re-iterate what Niklas has said at coala/coala-bears#1943 (comment) , you need to be creating your own scenarios which test different valid values, even if there is no defined style with that scenario of values.

@@ -14,18 +14,19 @@


class DocBaseClassTest(unittest.TestCase):
BaseClass = DocBaseClass()
Copy link
Member

Choose a reason for hiding this comment

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

this should be in method setUpClass or setUp .

@@ -19,6 +19,7 @@ class DocumentationCommentTest(unittest.TestCase):
ReturnValue = DocumentationComment.ReturnValue

Metadata = DocstyleDefinition.Metadata
BaseClass = DocBaseClass()
Copy link
Member

Choose a reason for hiding this comment

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

this (and the lines above) should be in method setUpClass or setUp

def func():
pass

## Documentation for a class.
#
# More details.

Copy link
Member

Choose a reason for hiding this comment

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

nope; wrong per https://www.stack.nl/~dimitri/doxygen/manual/docblocks.html#pythonblocks

(and also this fixup must undo the bad changes in the previous patch to default.py : #4549 (review) )

@jayvdb
Copy link
Member

jayvdb commented Jul 27, 2017

So for python I believe is that PEP-257 is old.

No, you are wrong. On https://www.python.org/dev/peps/pep-0257/ , you can see it's status is "Active". It has not been replaced. It is maintained at https://github.com/python/peps/commits/master/pep-0257.txt
And https://github.com/pycqa/pydocstyle implements it. And there are vibrant discussions about the finer details of it, and pydocstyle allowing ignoring some of its rules to allow for alternative styles (and recently has a few extra rules to enforce other styles), but nobody dares say pep 257 is not the official style guide of Python.

And I feel what we use here at coala is much better(which is an enhanced inherited version of PEP - 257 standards).

Again, wrong.
What coala uses is not particularly good. The reason it is bad is because we have not undertaken a project to standardise the docstrings. No bears are enabled. The docstrings can contain anything. Only manual code review can catch problems. Your project is intended to solve that. Or we can work towards enabling PyDocStyleBear.
There is also a bug in QuotesBear which imposes a silly PEP 257 violation.
In fact Stefan and I were complaining about the bears bad behaviour recently on another patch very recently. We were asking for changes without realising that QuotesBear prohibited it.

atm we don't have any mechanism to describe the docstring type.

A design problem, for sure.

@@ -9,12 +9,9 @@ def foobar_explosion(radius):
"""
A nice and neat way of documenting code.
:param radius: The explosion radius. """


def get_55():
"""A function that returns 55."""

Copy link
Member

Choose a reason for hiding this comment

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

this blank line is also not acceptable.

@@ -73,7 +68,6 @@ def foobar_triangle(side_A, side_B, side_C):
:return: returns perimeter
"""

Copy link
Member

Choose a reason for hiding this comment

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

this blank line is also not acceptable.

@@ -43,7 +39,6 @@ def best_docstring(param1, param2):
Cut to the Next Line.
"""

Copy link
Member

Choose a reason for hiding this comment

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

this blank line is also not acceptable.

start_index -= 1
while end_index < len(content) and content[end_index] == '\n':
# This condition will take place if theres an inline docstring
# following documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

uhhh, I still don't understand. The padding is 0

return self._function_padding

@property
def docstring_type_regex(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need getters for all those members?

Copy link
Member Author

Choose a reason for hiding this comment

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

As everything we are fetching from .coalang is a standard. And we don't want anyone to set those up manually. Hence setting them as getters :)
PS- Even the previous things like markers, comment's signature....etc we are fetching from .coalang are introduced in DocstyleDefinition as getters.

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, ok... since Python doesn't have private members, the whole concept is a bit useless. But if you want to do it like that it's fine.

*(str(sign) for sign in tuple(
docstyle_settings.get('docstring_type_regex', '')))
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

content, doc_comment_iterator)

doc_comment_iterator = DocBaseClass.instantiate_docstring_type(
content, doc_comment_iterator)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still unchanged and very inefficient. This method should be generator.

You have two options:

  1. Change instantiate_padding and instantiate_docstring_type to only process one docstring at a time, so this can become a generator.
  2. move the functionality from instantiate_padding and instantiate_docstring_type into extract_documentation_with_markers and use yield from

I prefer the second option.

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 I missed your previous comment we can do that...I see 👍

docstyle_settings['function_padding']
function_padding = cls.FunctionPadding(
*(int(padding) for padding in tuple(
docstyle_settings.get('function_padding', ''))))
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't have to use get with a default value, that's why you're in a try/except block. And why are you casting this to a tuple? isn't it already an iterable?

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 no need to use get now 🎉
yup this isn't an iterable. we need to cast into a tuple.

Copy link
Contributor

Choose a reason for hiding this comment

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

what is it then?

Copy link
Member Author

Choose a reason for hiding this comment

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

when this reads function_padding from .coalang the values are just read as a strings.
suppose function_padding = 1, 0
This will be read as
docstyle_settings = {..., function_padding : '1, 0',...}
we cast then into a tuple ('1', '0') so they get as iterables and then we typecast them into integers.

class_padding = cls.ClassPadding('', '')

try:
docstyle_settings['function_padding']
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this for?

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 we can remove this I guess...
as now I will use docstyle_settings['function_padding'] instead of docstyle_settings.get()
which will throw a KeyError 👍

Copy link
Member

Choose a reason for hiding this comment

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

this is what should throw the KeyError right?

This shouldn't be it, I was just showing an example

@@ -1,3 +1,5 @@
import re
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.

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
@@ -1,4 +1,3 @@
-import re
 
 from coalib.bearlib.languages.documentation.DocstyleDefinition import (
     DocstyleDefinition)

@damngamerz damngamerz force-pushed the blankline branch 2 times, most recently from 104815c to a641681 Compare August 9, 2017 12: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.

@damngamerz sorry this took so long. I'm beginning to like the overall design now 👍

Please have a look at my comments once more.

@property
def class_padding(self):
"""
A namedtuple consisting of values about blank lines before and after
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mention the name of all these namedtuples in the docstring. Here for example A namedtuple "ClassPadding" consisting..., etc

A namedtuple consisting of values about blank lines before and after
the documentation of ``docstring_type`` class.

These values are official standard of following blank lines before and
Copy link
Contributor

Choose a reason for hiding this comment

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

after missing

A namedtuple consisting of values about blank lines before and after
the documentation of ``docstring_type`` function.

These values are official standard of following blank lines before and
Copy link
Contributor

Choose a reason for hiding this comment

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

after missing

*(int(padding) for padding in tuple(
docstyle_settings['class_padding'])))
except IndexError:
class_padding = cls.ClassPadding('', '')
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the default '', '' and not 0, 0?

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 0, 0 will mean that official defined padding is 0, 0. So better leave the defaults to be '', ''

# Instantiate padding
top_padding = 0
bottom_padding = 0
start_index = doc.range.start.line - 2
Copy link
Contributor

Choose a reason for hiding this comment

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

please explain the -2 in a comment

bottom_padding += 1
end_index += 1
doc.top_padding = top_padding
doc.bottom_padding = bottom_padding
Copy link
Contributor

Choose a reason for hiding this comment

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

You could remove these two lines by replacing all occurrences of top_padding with doc.top_padding and dito for bottom_padding. There's no real need for a temporary variable.

end_index,
1 if bottom_padding > 0 else doc.range.end.column)

# Instantiate docstring_type
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is inaccurate, you're just compiling regexes here

end_index = doc.range.end.line

# We check for the class regex and function regex
# before and after the documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

def some_function():
  """
    documentation
  """
  class myPrivateClass:
     pass

This docstring would falsely be identified as a type class.

I suppose to mitigate this, you'll need to specify the position of the docstring in the coalang files, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so....we will require such a thing. Although I will add this in the test cases if it doesn't work we will have to think about the position.

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 this fails. We have two ways to solve this
First one is a tricky but will work, I think we can fix this by little prioritizing the algorithm
something like:

  1. we search the line closest to the docstring first. (whichever has least padding will be scanned by both the regexes first.)
  2. If paddings are found to be equal scanning the line before starting marker with regexes first(i.e. with both function and class). Than last line will be scanned.
    This will support the generic nature.

Second introducing docstring_type_position.
If we found its written top then we only scan top lines of the docstring with regexes or vice-versa.

Copy link
Member Author

Choose a reason for hiding this comment

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

Im going with the second option looks much more future proof 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

introducing docstring_type_position is the correct solution. The other will not work, you'll only make it work for Python. Maybe docstring_position should suffice, I doubt that styles will position the docstring on different sides for different types.

@@ -186,13 +238,38 @@ def load(cls, language: str, docstyle: str, coalang_dir=None):
metadata = cls.Metadata(*(str(docstyle_settings.get(req_setting, ''))
for req_setting in metadata_settings))

try:
class_padding = cls.ClassPadding(
*(int(padding) for padding in tuple(
Copy link
Contributor

Choose a reason for hiding this comment

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

In your previous comment you said that docstyle_settings['class_padding'] is a string like "1, 0", if this is true, tuple("1, 0") will be ("1", ",", " ", "0"), which int will choke on. Rather use docstyle_settings['class_padding'].split(",")

Also I'm wondering why this isn't caught by tests.

Copy link
Member Author

@damngamerz damngamerz Aug 10, 2017

Choose a reason for hiding this comment

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

Hmm now even I m wondering the same. You are correct about tuple('1,0') will be ("1", ",", " ", "0") but what I figured out is docstyle_settings is a Setting object. Now when I do tuple(docstyle_settings['class_padding'].value) gives me ("1", ",", " ", "0") but when I only do tuple(docstyle_settings['class_padding']) gives me ('1', '0')(some magic 😛 I m guessing it has to do something with the Setting object). So I think let it be that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

uh no, please investigate

Copy link
Member Author

Choose a reason for hiding this comment

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

I see why it's like that

class Setting(StringConverter):

Setting offers common data type conversions. It has list_delimeters for list/tuple conversion. That's why we are getting ('1', '0') while doing tuple(docstyle_settings['class_padding']) 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, talk about overengineering.... anyway... it's ok then


def test_DocBaseClass_instantiate_padding_inline_PYTHON3_7(self):
# To test that bottom_padding sets to nothing if docstring is
# followed by inline docstring.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this the desired behavior?

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 have such a case in DocStyleBear.
I don't wanna break the existing tests 😉
https://github.com/coala/coala-bears/blob/master/tests/documentation/test_files/DocumentationStyleBear/bad_file2.py.test#L21

Copy link
Contributor

Choose a reason for hiding this comment

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

how is such a case handled by documentation extraction?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is, if you parse and re-assemble this docstring, what will happen?

Copy link
Member Author

@damngamerz damngamerz Aug 10, 2017

Choose a reason for hiding this comment

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

oh yes! thats because....
The affected range is always taken from starting marker to the end marker(except in case of padding). DocumentationAPI doesn't care about whats after the end marker. Hence if its followed by inline comment...It's still is at the same position after the assembling.

@damngamerz damngamerz force-pushed the blankline branch 5 times, most recently from aed5f19 to eee38f9 Compare August 14, 2017 18:23
@NiklasMM
Copy link
Contributor

@damngamerz one last thing: The commit message needs an update: kind doesn't exist anymore

@damngamerz damngamerz force-pushed the blankline branch 2 times, most recently from 2457968 to a018577 Compare August 15, 2017 09:42
@damngamerz
Copy link
Member Author

@NiklasMM Updated commit message 👍

@damngamerz damngamerz force-pushed the blankline branch 2 times, most recently from 7f2f30c to d53307c Compare August 17, 2017 15:34
@NiklasMM
Copy link
Contributor

ack d53307c

Add `class_padding`, `function_padding` and
`docstring_position` in DocstyleDefinition
which are now acquired from `.coalang`. Add
`top_padding` and `bottom_padding` which is
automatically instantiated from
DocumentationExtraction. Add `docstring_type` in
DocumentationComment which will automatically
determine the type of docstring from
DocumentationExtraction. Add supporting test
cases.

Related to coala#4200
@SanketDG
Copy link
Member

ack 070a19e

@SanketDG
Copy link
Member

@rultor merge

@rultor
Copy link
Contributor

rultor commented Aug 18, 2017

@rultor merge

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

@rultor rultor merged commit 070a19e into coala:master Aug 18, 2017
@rultor
Copy link
Contributor

rultor commented Aug 18, 2017

@rultor merge

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

@damngamerz damngamerz deleted the blankline branch August 18, 2017 16:20
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.

7 participants