Skip to content

objtype: Set __doc__ as None if not included - #1550

Merged
coolreader18 merged 2 commits into
RustPython:masterfrom
seeeturtle:docstring
Oct 22, 2019
Merged

objtype: Set __doc__ as None if not included #1550
coolreader18 merged 2 commits into
RustPython:masterfrom
seeeturtle:docstring

Conversation

@seeeturtle

@seeeturtle seeeturtle commented Oct 20, 2019

Copy link
Copy Markdown
Contributor

This set __doc__ as None if it is not included in attribute
while making new instance of type.
In cpython if there is no docstring in class definition,
__doc__ attribute will be None. So RustPython should follow it.

Partially solves #1523

This only fixes class definition.

This set __doc__ as None if it is not included in attribute
while making new instance of type.
In cpython if there is no docstring in class definition,
__doc__ attribute will be None. So RustPython should follow it.

Partially solves RustPython#1523

@HyeockJinKim HyeockJinKim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should also write test code related to this code.

Comment thread vm/src/obj/objtype.rs
let mut attributes = dict.to_attributes();

// insert __doc__ as None if it is not included in attributes
if !attributes.contains_key("__doc__") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

>>>>> def f():
.....  pass
..... 
>>>>> f.__doc__
'The most base type'

The current code does not check the code above.
The expected result is shown below.

>>> def f():
...  pass
... 
>>> f.__doc__
>>> 

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, this doesn't fixes the same problem in function.
I'm planning to handle that in another PR.

This only fixes class definition.

@coolreader18 coolreader18 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants