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.

Sign up for GitHub

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

Jump to bottom

DOC: Code style documentation #30129

Merged
merged 8 commits into from
Jan 3, 2020
Merged

DOC: Code style documentation #30129

merged 8 commits into from
Jan 3, 2020

Conversation

ShaharNaveh
Copy link
Member

@ShaharNaveh ShaharNaveh commented Dec 7, 2019 •
edited
Loading

  • ref (comment)
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

This still needs a lot more work.
I will appreciate any advice/idea/fix.

ShaharNaveh
ShaharNaveh commented Dec 7, 2019
View reviewed changes
doc/source/development/contributing.rst Outdated
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)
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 need help linking the new file here.

Copy link
Member

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.

ShaharNaveh
ShaharNaveh commented Dec 7, 2019
View reviewed changes
doc/source/development/contributing_code_style.rst Outdated
@@ -0,0 +1,199 @@
.. _Not_sure_what_to_put_here:
Copy link
Member Author

Choose a reason for hiding this comment

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

Really not sure.

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 just name this contributing_code_guide and use that as the target in contributing.rst To address your question above as well

@alimcmaster1 alimcmaster1 added Docs Code Style Code style, linting, code_checks labels Dec 7, 2019
WillAyd
WillAyd requested changes Dec 7, 2019
View reviewed changes
doc/source/development/contributing_code_style.rst Outdated Show resolved Hide resolved
doc/source/development/contributing_code_style.rst Outdated Show resolved Hide resolved
doc/source/development/contributing_code_style.rst Outdated
@@ -0,0 +1,199 @@
.. _Not_sure_what_to_put_here:
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 just name this contributing_code_guide and use that as the target in contributing.rst To address your question above as well

datapythonista
datapythonista reviewed Dec 29, 2019
View reviewed changes
Copy link
Member

@datapythonista datapythonista left a 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.

doc/source/development/contributing_code_style.rst Outdated Show resolved Hide resolved
doc/source/development/contributing_code_style.rst Outdated Show resolved Hide resolved
doc/source/development/contributing_code_style.rst Outdated Show resolved Hide resolved
doc/source/development/contributing_code_style.rst Outdated

*pandas* uses 'repr()' instead of '%r' and '!r'.

The use of 'repr()' will only happend when the value is not an obvious string.
Copy link
Member

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.

doc/source/development/contributing_code_style.rst Outdated
-------------

*pandas* uses 'type(foo)' instead 'foo.__class__' as it is making the code more
readable.
Copy link
Member

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__.

Copy link
Member Author

Choose a reason for hiding this comment

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

#29775 (comment)

doc/source/development/contributing.rst Outdated
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)
Copy link
Member

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.

@ShaharNaveh ShaharNaveh force-pushed the DOC-doc-source-development-code-style branch 3 times, most recently from 6774098 to 03d3147 Compare December 29, 2019 14:57
datapythonista
datapythonista approved these changes Dec 29, 2019
View reviewed changes
Copy link
Member

@datapythonista datapythonista left a 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?

@ShaharNaveh ShaharNaveh marked this pull request as ready for review December 29, 2019 15:16
datapythonista
datapythonista reviewed Dec 29, 2019
View reviewed changes
doc/source/development/contributing_code_guide.rst Outdated

class MyClass:
def __init__(self):
cls = type(self)
Copy link
Member

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.

@datapythonista datapythonista changed the title WIP/DOC: Code style documentation DOC: Code style documentation Dec 29, 2019
@datapythonista
Copy link
Member

Thanks @MomIsBestFriend

@WillAyd can you have a look and see if your comments were addressed please?

@simonjayhawkins simonjayhawkins mentioned this pull request Dec 29, 2019
Closed
@datapythonista
Copy link
Member

@MomIsBestFriend can you have a look at the CI please? Seems some things are failing.

@jbrockmendel
Copy link
Member 

 Linting code-blocks in .rst documentation
##[error]doc/source/development/contributing_code_guide.rst:59:10:F821:undefined name 'new_function_name'
##[error]doc/source/development/contributing_code_guide.rst:59:21:F821:undefined name 'old_function_name'
##[error]doc/source/development/contributing_code_guide.rst:69:10:F821:undefined name 'new_function_name'
##[error]doc/source/development/contributing_code_guide.rst:69:21:F821:undefined name 'old_function_name'

@ShaharNaveh ShaharNaveh force-pushed the DOC-doc-source-development-code-style branch from b33d850 to 02c0ab5 Compare December 31, 2019 11:02
@ShaharNaveh
Copy link
Member Author

I have no idea how to solve it

https://github.com/pandas-dev/pandas/commit/02c0ab52d13dc00623bdf58a08bc51bc5818a2a1/checks?check_suite_id=379658620#step:6:2335

@datapythonista
Copy link
Member

I have no idea how to solve it

You can add the new page to this index. So, it's visible, and the error should also disappear.

@datapythonista
Copy link
Member

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

@ShaharNaveh
Copy link
Member Author

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?

@datapythonista
Copy link
Member

Up to you, but I think it'd make the page more visible if it's in that index.

@ShaharNaveh
Copy link
Member Author

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 contributing_code_guide under developer for example?

(and remove the doctree in contributing.rst?)

@datapythonista
Copy link
Member

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

@ShaharNaveh
Copy link
Member Author

I'm considering changing the file name to style or something like that, to make it one word, instead of three.

@ShaharNaveh
Copy link
Member Author

I guess you added an index because of the error

Correct.

@ShaharNaveh
Copy link
Member Author

I hope I did it correct.

@datapythonista
Copy link
Member

I'm considering changing the file name to style or something like that, to make it one word, instead of three.

I think style may be a bit ambiguous with other parts of the docs. But renaming to code_style sounds like a great idea.

datapythonista
datapythonista approved these changes Jan 1, 2020
View reviewed changes
Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @MomIsBestFriend

@datapythonista
Copy link
Member

@WillAyd can you have a look please? This should be ready

@WillAyd WillAyd merged commit f937843 into pandas-dev:master Jan 3, 2020
@WillAyd
Copy link
Member

WillAyd commented Jan 3, 2020

Thanks @momisbestfriend

@ShaharNaveh ShaharNaveh deleted the DOC-doc-source-development-code-style branch January 5, 2020 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd approved these changes

@datapythonista datapythonista datapythonista approved these changes

Assignees
No one assigned
Labels
Code Style Code style, linting, code_checks Docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ShaharNaveh @datapythonista @jbrockmendel @WillAyd @alimcmaster1