-
Notifications
You must be signed in to change notification settings - Fork 33
chore: update pre-commit #199
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
Changes from 3 commits
396748e
1e0cca8
5849ebf
62d854b
5bc77ca
bd8ab53
85910ec
b047974
0a16453
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,6 @@ | |
| Not to be included with code distributions. | ||
| """ | ||
|
|
||
|
|
||
| import math | ||
| import re | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| * No News Added: fix CI badge and target in README.rst | ||
|
|
||
|
|
||
| **Changed:** | ||
|
|
||
| * <news item> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| **Added:** | ||
|
|
||
| * No news needed: updating docformatter not user facing | ||
|
|
||
| **Changed:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Deprecated:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Removed:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Fixed:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Security:** | ||
|
|
||
| * <news item> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,6 @@ | |
| * SymmetryError | ||
| """ | ||
|
|
||
|
|
||
| import os | ||
| import sys | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,7 +152,7 @@ class P_cif(StructureParser): | |
| del k | ||
|
|
||
| BtoU = 1.0 / (8 * numpy.pi**2) | ||
| """float: Conversion factor from B values to U values.""" | ||
| """Float: Conversion factor from B values to U values.""" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @danielsirakov @stevenhua0320 we don't want string literals floating around in the code, so whenever we find things like this we want to clean them up. The correct fix in general is to move them to the docstring at the top of the function and put them into a standard format. Here, this may just be a constant and this should be changed from a string to a comment and placed above line 154.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, I cleaned up most of them, but I just now realized there are a few that I missed so I'll go back to address them as well |
||
|
|
||
| def _tr_ignore(a, value): | ||
| return | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,10 +135,10 @@ class P_pdb(StructureParser): | |
| "MASTER", | ||
| "END", | ||
| ] | ||
| """list: Ordered list of PDB record labels.""" | ||
| """List: Ordered list of PDB record labels.""" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. another floating string literal. It is documenting the static data list. Gemini suggested that even though it is not declared as self. it is still considered as a class attribute and so should be documented in the class docstring, something like: We can discuss this more, but in general I think taking this advice and using it also in other places in the PR is the right approach
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, I followed your format for this case and I will make sure to move other similar string literals I find to be documented in the class docstring |
||
|
|
||
| validRecords = dict.fromkeys(orderOfRecords) | ||
| """dict: Dictionary of PDB record labels.""" | ||
| """Dict: Dictionary of PDB record labels.""" | ||
|
|
||
| def __init__(self): | ||
| StructureParser.__init__(self) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,9 +57,11 @@ class P_vesta(StructureParser): | |
| """ | ||
|
|
||
| cluster_boundary = 2 | ||
| """int: Width of boundary around corners of non-periodic cluster. | ||
| Retained from the original AtomEye/XCFG parser for API compatibility. | ||
| VESTA handles periodicity natively so this value has no effect on output. | ||
| """Int: Width of boundary around corners of non-periodic cluster. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make standard |
||
|
|
||
| Retained from the original AtomEye/XCFG parser for API | ||
| compatibility. VESTA handles periodicity natively so this value has | ||
| no effect on output. | ||
| """ | ||
|
|
||
| def __init__(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.
this code seems just to be setting defaults, so the default values just have to be moved up to the attribute description in the docstring above and deleted from here, something like that.
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.
Here is the format I've been using for moving defaults to the attribute description, let me know if this is fine or if I should go back and change it:
Uh oh!
There was an error while loading. Please reload this page.
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.
Descriptions always start with "The". Otherwise, this is good.
Uh oh!
There was an error while loading. Please reload this page.
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.
Would this be fine for all:
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.
Yes, this is right, you need to inspect the edits and whereever the description of the variable is not starting with "The", make a change for that. Thanks!
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 went in and quickly changed all descriptions in the "Attributes" docstrings in diffpy.structure to be in that format. I could also look more thoroughly and update the format for all descriptions in the repository if you'd like