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

Coding Style Guidline.rst #34608

Closed
wants to merge 3 commits into from
Closed

Coding Style Guidline.rst #34608

wants to merge 3 commits into from

Conversation

Stockfoot
Copy link

Created a unified version of the Coding Style Guidelines
Extracted rules from various pandas website information as well as submission scripts that verify style requirements
Work in progress

Created a unified version of the Coding Style Guidelines
Extracted rules from various pandas website information as well as submission scripts that verify style requirements
Work in progress
@WillAyd
Copy link
Member

WillAyd commented Jun 9, 2020

This just duplicates what is already on the docs right? If so I think can just update the existing contributing guide with links rather than a new file with copied content

@Stockfoot
Copy link
Author

This just duplicates what is already on the docs right? If so I think can just update the existing contributing guide with links rather than a new file with copied content

This doesn't just copy what is in the docs.

This is just the start of the doc and I did pull information scattered in multiple locations about coding guidelines. I also started to pull out checks that are done in the script that check for coding guidelines that are not described anywhere.

If this document becomes more robust it think that it would be a useful tool for the OSS project.

@Stockfoot
Copy link
Author

I am also unsure of how to fix the CI/Checks error. It is saying there is a trailing white space after all my lines which do no show up in my IDE

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

OK thanks. So for things that are enforced by black I don't think we need to go into details as the tool just does these things for you.

We still need to figure out how to manage this versus existing documentation we have, but if you can trim down by removing the black sections should make it easier to iterate

2.1 Line Length
~~~~~~~~~~~~~~~

Line length is restricted to 80 characters to promote readability.
Copy link
Member

Choose a reason for hiding this comment

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

We use a line length of 88


Line length is restricted to 80 characters to promote readability.

2.2 Indentation
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 this is enforced by black; no need to detail here

if x == 3
x = 10

2.3 Whitespaces Around Arithmetic Operators
Copy link
Member

Choose a reason for hiding this comment

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

same for comment around black


x=3+5/2

2.4 Missing White Spaces After Commas
Copy link
Member

Choose a reason for hiding this comment

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

same for black


myFunctionCall('a','b','c')

2.5 Header File
Copy link
Member

Choose a reason for hiding this comment

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

Is this a Python reference?

value = str
f"Unknown recived type, got: '{type(value).__name__}'"

4 Types
Copy link
Member

Choose a reason for hiding this comment

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

OK so this is already in the contributing guideline. If we want to move here then should remove from that so as not to duplicate

@Stockfoot
Copy link
Author

@WillAyd I'll trim those out and resubmit, thank you for your help!

@Stockfoot
Copy link
Author

@WillAyd I have updated those sections. Let me know if there is anything else that I could research/add

@Stockfoot
Copy link
Author

@WillAyd I am also unsure of how to get past the CI / Checks portion of the pull request. It is saying there is a trailing white space after every line but in my IDE this is not there.

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

I think the trailing whitespace might be a false positive from not having the code-block directives explicitly declared


**Good:**

::
Copy link
Member

Choose a reason for hiding this comment

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

Can you make these code-block:: python directives?

2 Patterns
----------

2.1 Header File
Copy link
Member

Choose a reason for hiding this comment

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

What is this in reference to? This isn't anything Python related right? I think should just remove

import numpy as np
import pandas as pd

4.1.2 Unused Imports
Copy link
Member

Choose a reason for hiding this comment

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

Since this is enforced by flake8 already I don't think need to call out here


maybe_primes: List[Union[int, None]] = []

4.1.1 Redundant Imports
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 strictly for writing documentation, but for general code style it is certainly OK to import these. I think can just remove this from this guide

Copy link
Author

Choose a reason for hiding this comment

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

@WillAyd I made those changes you requested, but it still seems like it is failing after ever carriage line return

@jbrockmendel
Copy link
Member

The section on repr can probably go, since we mostly use fstrings now.

@WillAyd
Copy link
Member

WillAyd commented Sep 10, 2020

@Stockfoot still active? If so can you simplify and address comments?

@Stockfoot
Copy link
Author

@WillAyd I am still around.
I am just unsure of how to proceed. The CI/Check seems to fail no matter what I do.
I have made all the changes that were previously suggested. The only one I need to address is the most recent with @jbrockmendel

The section on repr can probably go, since we mostly use fstrings now.

@github-actions
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Oct 12, 2020
Copy link
Member

@arw2019 arw2019 left a comment

Choose a reason for hiding this comment

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

@Stockfoot are you interested in continuing? If yes if you can address comments & merge master we'll take another look

@Stockfoot
Copy link
Author

@arw2019 I am interested in continuing. I have addressed the comments but no one has ever responded back to my question on how to proceed and actually get a pull request going.

Every time I try and submit it will no pass CI / Checks and am unsure how to proceed...

image

@arw2019
Copy link
Member

arw2019 commented Nov 5, 2020

Can you merge master again and I'll take a look (output from the last build is no longer available for inspection). Some failures are random but we want to make sure otherwise will have to address

Also I might be wrong but I don't think you responded to https://github.com/pandas-dev/pandas/pull/34608/files#r440389770

@arw2019
Copy link
Member

arw2019 commented Nov 27, 2020

Closing for now. @Stockfoot let us know whenever you'd like to pick this up and we'll reopen

@arw2019 arw2019 closed this Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: Single Document For Code Guidelines
5 participants