Skip to content

objtype: Set __doc__ as None if not included #1550

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

Merged
merged 2 commits into from
Oct 22, 2019

Conversation

seeeturtle
Copy link
Contributor

@seeeturtle seeeturtle commented Oct 20, 2019

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
Copy link
Contributor

@HyeockJinKim HyeockJinKim left a comment

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.

let mut attributes = dict.to_attributes();

// insert __doc__ as None if it is not included in attributes
if !attributes.contains_key("__doc__") {
Copy link
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
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.

Copy link
Member

@coolreader18 coolreader18 left a comment

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