update isort, flake8 and black to set line length limit to 79 #324
Conversation
|
@sbillinge It's ready for review. |
There was a problem hiding this comment.
Thanks for this amazing work @ycexiao !
Please could you make sure to handle the long-lines in the JSON according to how I suggested in the comment in the open issue (#320 ). I don't want to go through the code to find it, so preferrably when it is done just paste it into a comment.
then when pre-commit passes with that change I can merge.
Also, please confirm in a comment that now functional code was changed anywhere, just docstrings, comments and layout of imports. thanks so much.
tests/conftest.py
Outdated
| "The two objects have different values in x arrays " | ||
| "(my_do.all_arrays[:, [1, 2, 3]]). Please ensure the x values of the " | ||
| "two objects are identical by re-instantiating the DiffractionObject " | ||
| "with the correct x value inputs. " |
There was a problem hiding this comment.
you appear to left a blank space at the end of the string. Please remove this everywhere. The logic of the blank space is so that when the lines are combined by Python to remake the long string two words do not become concatanated likethis. But it is not desired to have a blank space at the very end of the string that is not at a line-break.
Please can you fix this everywhere?
| [ | ||
| # Test addition, subtraction, multiplication, and division of two DO objects | ||
| ( | ||
| "operation, expected_do_1_all_arrays_with_y_modified, " |
There was a problem hiding this comment.
this line-break has broken the code. Because this is an argument being passed into the test function we need to break it more carefully, something like
"operation, expected_do_1_all_arrays_with_y_modified, \
expected_do_2_all_arrays_with_y_modified",
if this doesn't work and allow tests to pass (you are running tests before pushing, right?) then we can give this the # noqa treatment also.
There was a problem hiding this comment.
I found that it was caused a misplaced comma before the right parenthesis. I think it would be better to stick to the previous convention, i.e.
from
(
"operation, expected_do_1_all_arrays_with_y_modified, "
"expected_do_2_all_arrays_with_y_modified",
)[
....
]to
(
"operation, expected_do_1_all_arrays_with_y_modified, "
"expected_do_2_all_arrays_with_y_modified"
),
[
....
]
|
Thank you @sbillinge. I will fix the white space, set flake8 to ignore warnings from necessary long line, and double-check with running the tests. |
|
@ycexiao I can't merge a branch with a force-push. What probably happened there is that pre-commit updated the code on your fork and you didn't synchronize that before doing more edits. This isn't avoided but is minimized by making sure pre-commit passes locally before pushing. Also, these changes caused a bunch of tests to fail, so we will have to go more slowly. Please make sure you are running both pre-commit and pytest locally and getting them to pass before pushing to a PR. Normally I like PRs early so I can give fieedback, but in this case you won't be able to merge until pre-commit is passing and so all the lines are short enough, but also we need tests to pass. When you make tests pass it is VERY important to understand whether it is the test at fault or the code being tested. This means that when you do the line-shortening,, run the test on that code module and make sure it passes, then edit the error message (for example in the code) and make sure the test still passes, then edit the test until it passes again. Please confirm you understand this order and why it should be this way. |
|
Thank you @sbillinge. My understanding is (1) avoid force push Previously, I didn't know that we were using pytest, what I did was runnig I have one additional question about (1) which is related to the group PR practice. In this case, the former PR is failed. I want to modify the previous commit in this PR (make different shortenings to pass pytest). I found that If I want to keep the PR comment, I have to use the original PR, and if I want to use the original PR, the local commit will conflict with the original one, so there must be a 'merge' commit unless I use 'force push'. I thought the 'merge' commit to the wrong PR branch was redundant, so I used 'force push.' If 'force push' is not allowed, what should I do then? Delete the current one and open a new one ? |
You can just push new edits to the same PR. This is the normal workflow. As I discussed before, the conflict probably arose because an edit happened remotely and your local was not resynched with your fork. This can happen when CI runs black etc. (1) To minimize these effects make sure it is paying pre-commit locally before pushing to aPR (2) if it happens, sync your local by pulling the branch from origin before committing again. If that doesn't work you can always close the PR and make a new one. |
Thank you. What I want to do here is to avoid including inappropriate commit histories in the PR. This means modifying the commits already in the cloud, so there have to be conflicts. But from your words here, I can understand the general picture. I will open a new PR when I want to do this next time. |
We would like cleaner commit histories in general, but we don't want to modify commit histories because it breaks our ability to go back to earlier versions when we need to. This is the major power of git, that we can recover every version of every file. So we don't allow force pushes. But if the commit history gets very dirty, we just kill the PR and do it over. |
|
Please see #326 |
close #320