Add a paragraph on bad file names to the style guide #1439
Conversation
github-actions
bot
added
the
documentation
docs/styleguide/PyGuide.md
Outdated
| @@ -965,6 +965,11 @@ Always use a `.py` filename extension. Never use dashes. | |||
| Python filenames must have a `.py` extension and must not contain dashes (`-`). | |||
| This allows them to be imported and unit tested. | |||
|
|
|||
| Avoid file and module names of `utils`, `helpers` etc. and do not extend existing files and packages with these names. | |||
There was a problem hiding this comment.
I don't see this is an industry standard. Here are some examples:
There was a problem hiding this comment.
In all of these examples the utils part can either be removed without loss of semantic meaning, or replaced to a more fitting name (e.g. the transformers example seems to contain mostly test and CI scripts and checkers - there are your two-three names (tests, ci_checks, checks) that could be used instead of utils.
There was a problem hiding this comment.
PyTorch contains quite a lot of stuff in utils moving to torch namespace will make it messy. The content of Keras also disapproves of your statement.
How does logging fit the common namespace in your case? The name common should also fall into the rule you proposed in this PR.
There was a problem hiding this comment.
No, not really messier than it already is. I don't see how torch.utils.jit.log_extract from their code is better kept in torch.utils.jit rather than in the already existing torch.jit, and why torch.jit._decomposition_utils is not torch.utils.jit._decomposition. As for Keras - I'm sure that bad decisions can be found in any code base, Google's is no exception.
As mentioned, common is a descriptive name in a sense that it separates the backend-agnostic code and definitions from the backend-specific code. I can add a README.md file to that module like I did in tests/common and tests/shared to give a more documental definition for this.
There was a problem hiding this comment.
Some point from https://maestros.io/whats-in-a-utils-file
I came across to a utils.js -or something like that- it was a real headache. I had to guess what's in there.
I agree that avoid utils and helpers names is good practices. Put code to utils it's the easiest way, better find correct names that will display the functionality. But don't need to strictly follow this, otherwise there will be many small files.
There was a problem hiding this comment.
For example my PR https://github.com/openvinotoolkit/nncf/pull/1404/files, for me several function can be added to utils.py. But it's can be already discussed.
There was a problem hiding this comment.
No, not really messier than it already is. I don't see how
torch.utils.jit.log_extractfrom their code is better kept intorch.utils.jitrather than in the already existingtorch.jit, and whytorch.jit._decomposition_utilsis nottorch.utils.jit._decomposition. As for Keras - I'm sure that bad decisions can be found in any code base, Google's is no exception.As mentioned,
commonis a descriptive name in a sense that it separates the backend-agnostic code and definitions from the backend-specific code. I can add a README.md file to that module like I did intests/commonandtests/sharedto give a more documental definition for this.
Common can be descriptive for backend-agnostic code but logging is not related to backends at all. I recommend not mixing the repository folder layout and import layout because these are two different things. The former is our internal reasonable preferences while the latter is what and how we expose this to the user.
The initially proposed guidance does not make sense to me. @alexsu52, proposed a different rule of thumb in the style guide, and I support it.
There was a problem hiding this comment.
backend-agnostic code but logging is not related to backends at all
Is backend-agnostic code related to backends, and if so, how is this different from how logging is related to backends?
I recommend not mixing the repository folder layout and import layout because these are two different things.
Good luck doing this in Python without code-generation and importlib magic.
The initially proposed guidance does not make sense to me. @alexsu52, proposed a different rule of thumb in the style guide, and I support it.
@alexsu52's wording is a subset of mine. Mine is stronger and also gives a description of consequences and of what to do instead of the wrong behaviour. I stand by my wording.
There was a problem hiding this comment.
I have update the paragraph to integrate @alexsu52's wording, shortened it somewhat and added examples of good and bad layouts. I also kept a short sentence on what to do instead, so that the recommendation is not just a plain prohibition but also describes a course of action.
docs/styleguide/PyGuide.md
Outdated
| Avoid file and module names of `utils`, `helpers` etc. and do not extend existing files and packages with these names. | ||
| Otherwise there is always a temptation to add an additional piece of code to this file/module, which ends up in having large files with unrelated functions and classes. | ||
| Instead group your new code in dedicated files/modules that are named explicitly according to the purpose of code - either create new files/modules for this or extend the already existing, properly named ones. | ||
| If you see an opportunity to break down a `utils.py` or `helpers.py` file into a better named and grouped set of files - take it. |
There was a problem hiding this comment.
| Avoid file and module names of `utils`, `helpers` etc. and do not extend existing files and packages with these names. | |
| Otherwise there is always a temptation to add an additional piece of code to this file/module, which ends up in having large files with unrelated functions and classes. | |
| Instead group your new code in dedicated files/modules that are named explicitly according to the purpose of code - either create new files/modules for this or extend the already existing, properly named ones. | |
| If you see an opportunity to break down a `utils.py` or `helpers.py` file into a better named and grouped set of files - take it. | |
| Avoid creating Python files like a "swiss knife" by naming them with common names such as `utils.py`, `helpers.py`, `common.py` and etc. |
There was a problem hiding this comment.
Rewrote the paragraph, please re-review.
There was a problem hiding this comment.
Let's handle *.py at the first.
vshampor
force-pushed
the
style_adjustments
branch
from
8b18c25 to
0813b52
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #1439 +/- ##
===========================================
- Coverage 89.86% 85.94% -3.92%
===========================================
Files 485 485
Lines 44491 43744 -747
===========================================
- Hits 39981 37596 -2385
- Misses 4510 6148 +1638
Flags with carried forward coverage won't be shown. Click here to find out more. |
No description provided.