-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
DOC: Code style documentation #30129
DOC: Code style documentation #30129
Conversation
Additional standards are outlined on the `code style wiki | ||
page <https://github.com/pandas-dev/pandas/wiki/Code-Style-and-Conventions>`_. | ||
Additional standards are outlined on the (Placeholder as a reminder to ask the | ||
devs for the help on how to properly link the new created file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need help linking the new file here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remember the exact syntax, but you have plenty of examples of links to labels in the docs.
@@ -0,0 +1,199 @@ | |||
.. _Not_sure_what_to_put_here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just name this contributing_code_guide
and use that as the target in contributing.rst
To address your question above as well
@@ -0,0 +1,199 @@ | |||
.. _Not_sure_what_to_put_here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just name this contributing_code_guide
and use that as the target in contributing.rst
To address your question above as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MomIsBestFriend if you can address the comments and leave this ready to get merged please.
|
||
*pandas* uses 'repr()' instead of '%r' and '!r'. | ||
|
||
The use of 'repr()' will only happend when the value is not an obvious string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is true. In some cases it makes more sense to represent the object as a string, and in some as its representation. See:
>>> f'today is {my_date}'
'today is 2020-01-01'
>>> f'the object {my_date!r} is not a string'
'the object datetime.date(2020, 1, 1) is not a string'
Better avoid saying when to use the representation, since this is more common sense, and focus on the formatting.
@jbrockmendel do we really prefer {repr(foo)}
to {foo!r}
. I think I've seen some discussion about this, but not sure this is really an standard we follow, and why the former should be preferred.
------------- | ||
|
||
*pandas* uses 'type(foo)' instead 'foo.__class__' as it is making the code more | ||
readable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are you getting this standards from? I don't see any of this in the wiki https://github.com/pandas-dev/pandas/wiki/Code-Style-and-Conventions, and I don't think this is an agreed practice, I usually use foo.__class__.__name__
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional standards are outlined on the `code style wiki | ||
page <https://github.com/pandas-dev/pandas/wiki/Code-Style-and-Conventions>`_. | ||
Additional standards are outlined on the (Placeholder as a reminder to ask the | ||
devs for the help on how to properly link the new created file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remember the exact syntax, but you have plenty of examples of links to labels in the docs.
6774098
to
03d3147
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MomIsBestFriend can you make this a non-Draft PR please?
@jbrockmendel can you have a look please?
|
||
class MyClass: | ||
def __init__(self): | ||
cls = type(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these examples are too complicated to illustrate the point here.
You can simply show something like:
foo = 'bar'
type(foo) # good
foo.__class__ # bad
I think creating classes, too many cases... creates noise, more than help understand the point.
Thanks @MomIsBestFriend @WillAyd can you have a look and see if your comments were addressed please? |
@MomIsBestFriend can you have a look at the CI please? Seems some things are failing. |
|
b33d850
to
02c0ab5
Compare
You can add the new page to this index. So, it's visible, and the error should also disappear. |
Sorry, I see I forgot to copy the link. Another option is to add the document to this index, instead of creating a new one: https://github.com/pandas-dev/pandas/blob/master/doc/source/development/index.rst |
This is your preferred way? |
Up to you, but I think it'd make the page more visible if it's in that index. |
Since you suggested it, I will go with that. Do I just put (and remove the |
Yep, just have your page as one more item in the list, not sure what's the best position, up to you. You can probably leave a regular link in contributing, I guess you added an index because of the error |
I'm considering changing the file name to |
Correct. |
I hope I did it correct. |
I think style may be a bit ambiguous with other parts of the docs. But renaming to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks @MomIsBestFriend
@WillAyd can you have a look please? This should be ready |
Thanks @momisbestfriend |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
This still needs a lot more work.
I will appreciate any advice/idea/fix.