New Coding Style guide #95
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgepiloto
force-pushed
the
feat/code-style
branch
from
d1c5f54 to
5b77345
Compare
|
Now that I am reviewing our "Code Style" section, I'm seeing lots of inconsistencies across our "avoid" and "use" cases. Fixing those in this PR. By the way, Sphinx tabs turned out to be very useful. |
|
The [blacken-docs] tool is very useful for this. I will add a reference to it in #85. |
jorgepiloto
force-pushed
the
feat/code-style
branch
4 times, most recently
from
884d96a to
100dfb4
Compare
PipKat
reviewed
May 16, 2022
PipKat
reviewed
May 16, 2022
PipKat
reviewed
May 16, 2022
PipKat
reviewed
May 16, 2022
PipKat
reviewed
May 16, 2022
PipKat
reviewed
May 16, 2022
PipKat
reviewed
May 16, 2022
PipKat
reviewed
May 16, 2022
PipKat
reviewed
May 16, 2022
jorgepiloto
force-pushed
the
feat/code-style
branch
from
1406138 to
012a375
Compare
jorgepiloto
force-pushed
the
feat/code-style
branch
from
012a375 to
cf6481b
Compare
PipKat
reviewed
May 20, 2022
PipKat
reviewed
May 20, 2022
PipKat
reviewed
May 20, 2022
PipKat
reviewed
May 20, 2022
PipKat
reviewed
May 20, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
| ~~~~~~~~~~~~~~~ | ||
| Imports should always be placed at the top of the file, just after any | ||
| module comments and docstrings and before module globals and | ||
| constants. This reduces the likelihood of an `ImportError`_ that |
There was a problem hiding this comment.
Suggested change
| constants. This reduces the likelihood of an `ImportError`_ that | |
| constants. This reduces the likelihood of an `ImportError`_ that |
PipKat
reviewed
May 23, 2022
Comment on lines
+51
to
+52
| Imports Order | ||
| ~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Suggested change
| Imports Order | |
| ~~~~~~~~~~~~~ | |
| Import Order | |
| ~~~~~~~~~~~~ |
PipKat
reviewed
May 23, 2022
| Import Namespaces | ||
| ~~~~~~~~~~~~~~~~~ | ||
| You should avoid using wildcards in imports because doing so can make it | ||
| difficult to detect undefined names. For more information, see `Python |
There was a problem hiding this comment.
Suggested change
| difficult to detect undefined names. For more information, see `Python | |
| difficult to detect undefined names. For more information, see `Python |
PipKat
reviewed
May 23, 2022
Comment on lines
+228
to
+230
| Protected methods can be called from inherited classes, | ||
| unlike private methods, which names are 'mangled' to avoid | ||
| these methods from being called from inherited classes. |
There was a problem hiding this comment.
Suggested change
| Protected methods can be called from inherited classes, | |
| unlike private methods, which names are 'mangled' to avoid | |
| these methods from being called from inherited classes. | |
| Protected methods can be called from inherited classes. | |
| For private methods, names are 'mangled' to prevent | |
| these methods from being called from inherited classes. |
PipKat
reviewed
May 23, 2022
|
|
||
| - Body of a control statement, such as a loop or a select statement | ||
| - Body of a conditional statement | ||
| - New scope block |
There was a problem hiding this comment.
Suggested change
| - New scope block | |
| - New scope blocks |
PipKat
reviewed
May 23, 2022
|
|
||
|
|
||
| def top_level_function(): | ||
| """Top level function docstring.""" |
There was a problem hiding this comment.
Suggested change
| """Top level function docstring.""" | |
| """Top-level function docstring.""" |
PipKat
reviewed
May 23, 2022
Comment on lines
+427
to
+429
| However, an important portion of the behavior that is not self-apparent | ||
| should include a note from the developer writing it. Otherwise, | ||
| future developers may remove what they see as unnecessary. |
There was a problem hiding this comment.
Suggested change
| However, an important portion of the behavior that is not self-apparent | |
| should include a note from the developer writing it. Otherwise, | |
| future developers may remove what they see as unnecessary. | |
| However, if code behavior is not self-apparent, it should be documented. | |
| Otherwise, future developers might remove code that they see as unnecessary. |
PipKat
reviewed
May 23, 2022
Comment on lines
+504
to
+507
| The following sections provide some `PEP8 | ||
| <https://www.python.org/dev/peps/pep-0008/>`_ suggestions for removing | ||
| ambiguity and preserving consistency. They also address some common pitfalls | ||
| when writing Python code. |
There was a problem hiding this comment.
Suggested change
| The following sections provide some `PEP8 | |
| <https://www.python.org/dev/peps/pep-0008/>`_ suggestions for removing | |
| ambiguity and preserving consistency. They also address some common pitfalls | |
| when writing Python code. | |
| The following sections provide some `PEP8 | |
| <https://www.python.org/dev/peps/pep-0008/>`_ recommendations for removing | |
| ambiguity and preserving consistency. Additionally, they address some common pitfalls | |
| that occur when writing Python code. |
PipKat
reviewed
May 23, 2022
| def test_fahr_to_kelvin(): | ||
| np.testing.assert_allclose(12.7778, fahr_to_kelvin(55)) | ||
|
|
||
| Now, you have only one line of code to verify and can also use |
There was a problem hiding this comment.
Suggested change
| Now, you have only one line of code to verify and can also use | |
| Now, you have only one line of code to verify. You can also use |
PipKat
reviewed
May 23, 2022
|
|
||
| If the loop is too complicated for creating a list comprehension, | ||
| consider creating small functions and calling these instead. For | ||
| example to extract all consonants in a sentence: |
There was a problem hiding this comment.
Suggested change
| example to extract all consonants in a sentence: | |
| example, to extract all consonants in a sentence: |
PipKat
reviewed
May 23, 2022
| Security, an ongoing process involving people and practices, ensures application confidentiality, integrity, and availability [#]_. | ||
| Any library should be secure and implement good practices that avoid or mitigate possible security risks. | ||
| This is especially relevant in libraries that request user input (such as web services). | ||
| Because security is a broad topic, we recommend you review this useful Python-specific resource: |
There was a problem hiding this comment.
Suggested change
| Because security is a broad topic, we recommend you review this useful Python-specific resource: | |
| Because security is a broad topic, you should review this useful Python-specific resource: |
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
reviewed
May 23, 2022
PipKat
approved these changes
May 23, 2022
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
|
Fixing some code suggestions in a separate PR. Those didn't appear in batch mode. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partially solves for #64.
To be merged after #85.