Checks: Fix Flake8 E501

[mshukuno]

E501 - line too long

• Docstrings (descriptions and parameters) - fixed by using the existing format (authors' docstrings - length, parameters aligned or x indentations, etc), most of which are shorter than 88 characters. When no docstring style was present in the files, the Black line length was used. Since Black doesn't support formatting docstrings and comments (GH#2865), this work is done manually to cross out all E501 issues. If a uniform docstring format is preferred, the docformatter may be used. I've tried a few and it didn't modify "code-blocks" or "doctest (>>>)".
• Several files contain "code-blocks" or "doctest (>>>)" that exceed the Black character length limit. In this case, # noqa: E501 was added to the end of docstrings to ignore issues which is good.
• Regex strings - # noqa: E501
• Jinja2 HTML template strings - # noqa: E501
• Strings and comments were intended to be formatted in a specific way: For example, comments were used as measuring tape for string lengths - # noqa: E501

About ignoring errors with flake8

Q. Is it possible to ignore a function with # noqa? Answer: No, I think. A function can be ignored with it according to this website . However, it didn't work for me. The flake8 documentation does not include function ignore examples, but rather lines and entire files. There may have been a version of flake8 that could do it in the past.

---

April 2024

Continued fixing E501 (line too long).

• Docstrings and comments exceeding 88 lines are reformatted.
• Used blackdoc instead of Ruff since Ruff didn't format some cases. The blackdoc works similar to black, but for doctest. A double quote is enforced as well as code formatting. Because blackdoc worked so well, I didn't have to manually reformat the doctest code. The doctest outputs are left alone so some output lines exceeded 88 lines. In such cases, a # noqa: E501 was added.
• There are a variety of formats for commented code. Keeping within the line limit was the goal. Although some of them were reformatted as black.
• Print statements - The print line statements like below weren't formatted. Ignored with # noqa: E501

print(
     " +-------------------- Temporal Topology -------------------------------------+"  # noqa: 
)

• Regex - Keeping as is with # noqa: E501.
• URL - Most URLs kept as they are with # noqa: E501.
• HTML - Found HTML string templates (some formatted, some # noqa: E501, and one flake8 configuration) and jinja2 (# noqa: E501).
• WXGUI (e.g. %module) - Added to .flake8 configuration. Q: Is it possible to have multiple lines? If so, please provide me with instructions. Since it will appear in the GUI, adding # noqa isn't an option.

Notes:

• Two types of docstring format, Epytext (@param param1: xxx) and reST (:param param1: xxx).
• Multiline parameter descriptions - There were many styles. It would have been easier for me if there had been a guideline.
  • Case 1: Align the beginning of the description
  • Case 2: With 4 or 8 or more indents

[echoix]

I'm wondering if you'd have some more luck with ruff for the docstrings/doctest.

[echoix]

For black, I've been waiting for a dependent PR before https://github.com/OSGeo/grass/pull/3538, and now waiting for that one, and right when it is merged, we'll be updating the black version, after skipping them for 2 years

[mshukuno]

I'm wondering if you'd have some more luck with ruff for the docstrings/doctest.

I tried "ruff" to format "code blocks/doctests" and it wasn't perfect. There's still some manual work to do on E501. But it reduces E501 significantly. Let me know if you want them formatted instead of placing # noqa: E501.

[echoix]

If you are able to fix them without disabling the errors it would be perfect!

Tomorrow I'll try updating Black's version just like https://github.com/OSGeo/grass-addons/pull/1046 (unless you'd like to do it before that). With the recent Black version, there shouldn't be a big difference, if any, of running ruff format. It might help you out (I imagine you undo the changes that aren't in the doctests?)

[mshukuno]

If you are able to fix them without disabling the errors it would be perfect!

Tomorrow I'll try updating Black's version just like OSGeo/grass-addons#1046 (unless you'd like to do it before that). With the recent Black version, there shouldn't be a big difference, if any, of running ruff format. It might help you out (I imagine you undo the changes that aren't in the doctests?)

Update Black version tomorrow like you planned. I started fixing flake8 errors from the "python" directory to see how error fixing goes (starting small) since I am new to this. Please let me know if you or another reviewer have any suggestions for PR for this issue (e.g. all fixes for an error or all fixes for a directory, etc...).

[echoix]

If you are able to fix them without disabling the errors it would be perfect!

Tomorrow I'll try updating Black's version just like OSGeo/grass-addons#1046 (unless you'd like to do it before that). With the recent Black version, there shouldn't be a big difference, if any, of running ruff format. It might help you out (I imagine you undo the changes that aren't in the doctests?)

Update Black version tomorrow like you planned. I started fixing flake8 errors from the "python" directory to see how error fixing goes (starting small) since I am new to this. Please let me know if you or another reviewer have any suggestions for PR for this issue (e.g. all fixes for an error or all fixes for a directory, etc...).

What I found that works best here is to keep the PRs relatively small in complexity, but can apply to every instance in the code. It's better keep the scope small enough so a single reviewer can go through without too many questions. It's still hard here to have multiple reviewers for complimentary skillsets joining themselves to review a PR. So, if you make a certain type of change, and use that same logic everywhere, it's easy to review. If there are too much places where there is an ambiguity, and need more info/discussion, it can make the complete PR be delayed. So perharps a bigger PR with only trivial fixes, and a small one that contains changes that might be tricky.

Having too many unrelated changes interweaved in the code makes reading the diff a bit longer, since we need to think on the "why" for each one. But it is still possible when it is limited to a single file.

[echoix]

Here, about 400 changes dispersed about in 44 files is still manageable, but I think it's close to enough for this PR.

[petrasovaa]

What I found that works best here is to keep the PRs relatively small in complexity, but can apply to every instance in the code. It's better keep the scope small enough so a single reviewer can go through without too many questions. It's still hard here to have multiple reviewers for complimentary skillsets joining themselves to review a PR. So, if you make a certain type of change, and use that same logic everywhere, it's easy to review. If there are too much places where there is an ambiguity, and need more info/discussion, it can make the complete PR be delayed. So perharps a bigger PR with only trivial fixes, and a small one that contains changes that might be tricky.

Having too many unrelated changes interweaved in the code makes reading the diff a bit longer, since we need to think on the "why" for each one. But it is still possible when it is limited to a single file.

Yes, would it be possible to revert the changes unrelated to E501 (and create separate PRs)? This is getting difficult to review and I think we may have to discuss more some of the other changes, which would be best done in a different PR...

[mshukuno]

What I found that works best here is to keep the PRs relatively small in complexity, but can apply to every instance in the code. It's better keep the scope small enough so a single reviewer can go through without too many questions. It's still hard here to have multiple reviewers for complimentary skillsets joining themselves to review a PR. So, if you make a certain type of change, and use that same logic everywhere, it's easy to review. If there are too much places where there is an ambiguity, and need more info/discussion, it can make the complete PR be delayed. So perharps a bigger PR with only trivial fixes, and a small one that contains changes that might be tricky. Having too many unrelated changes interweaved in the code makes reading the diff a bit longer, since we need to think on the "why" for each one. But it is still possible when it is limited to a single file.

Yes, would it be possible to revert the changes unrelated to E501 (and create separate PRs)? This is getting difficult to review and I think we may have to discuss more some of the other changes, which would be best done in a different PR...

I am sorry. I didn't know that a push after PR could add commits to existing PR. Please feel free to revert and/or close the current PR to make reviews easier. Thank you for your patience as I learn how to use GitHub.

[echoix]

I am sorry. I didn't know that a push after PR could add commits to existing PR. Please feel free to revert and/or close the current PR to make reviews easier. Thank you for your patience as I learn how to use GitHub.

Here would be some steps that would help you. I'm assuming that all changes you have are already committed and pushed before starting. In your fork, on your branch on GitHub, go to the combo box where you can choose what branch to show. Enter the new name of a branch. Since it doesn't exist, GitHub will suggest to create a branch from the currently active commit. That's what you want. It will be the start of your next PR.

Find the SHA of any commit that is behind the commits that you want to separate from the branch of this PR.

Then, locally, with the branch of this PR checked out, run git fetch (and git pull if it isn't up to date). Then, git rebase -i (the commit SHA that you found) That will open a text editor in the console. When you save this, the contents that you edit will be what your branch will be. Since it's a vim editor, you need to press escape to start editing. Remove (backspace) the lines of the commits you want removed, or change the text "pick" of the first column to "drop" or "d". Once done, enter "escape" again, then :wq to save. Your commits will be removed from the branch. Since you made a backup branch, you won't have lost anything. You just completed a rebase locally! But to push it, since your locally branch is behind the one one GitHub, but you want this to be the current state, you need to force push. It is correct after a rebase to need to force push, but still have to be careful. The safest command and flags to use (that will stop you of doing some common mistakes, but not all of them), is

git push --force --force-if-includes --force-with-lease

You'll then have finished bringing back the PR branch to what you think it should be.

For your next PR, it should probably be rebased on top of the main branch of the repo once merged, (since it will contain commits for the two PRs), but it can probably be handled on our side through the GitHub interface.

[mshukuno]

I am sorry. I didn't know that a push after PR could add commits to existing PR. Please feel free to revert and/or close the current PR to make reviews easier. Thank you for your patience as I learn how to use GitHub.

Here would be some steps that would help you. I'm assuming that all changes you have are already committed and pushed before starting. In your fork, on your branch on GitHub, go to the combo box where you can choose what branch to show. Enter the new name of a branch. Since it doesn't exist, GitHub will suggest to create a branch from the currently active commit. That's what you want. It will be the start of your next PR.

Find the SHA of any commit that is behind the commits that you want to separate from the branch of this PR.

Then, locally, with the branch of this PR checked out, run git fetch (and git pull if it isn't up to date). Then, git rebase -i (the commit SHA that you found) That will open a text editor in the console. When you save this, the contents that you edit will be what your branch will be. Since it's a vim editor, you need to press escape to start editing. Remove (backspace) the lines of the commits you want removed, or change the text "pick" of the first column to "drop" or "d". Once done, enter "escape" again, then :wq to save. Your commits will be removed from the branch. Since you made a backup branch, you won't have lost anything. You just completed a rebase locally! But to push it, since your locally branch is behind the one one GitHub, but you want this to be the current state, you need to force push. It is correct after a rebase to need to force push, but still have to be careful. The safest command and flags to use (that will stop you of doing some common mistakes, but not all of them), is

git push --force --force-if-includes --force-with-lease

You'll then have finished bringing back the PR branch to what you think it should be.

For your next PR, it should probably be rebased on top of the main branch of the repo once merged, (since it will contain commits for the two PRs), but it can probably be handled on our side through the GitHub interface.

I appreciate you taking the time to explain this to me. Thank you so much! Creating a branch for each issue makes sense!!

[echoix]

Oops, I think you committed your IDE support files

[mshukuno]

Oops, I think you committed your IDE support files

My goodness... There is so much mess I am making. Is it OK or possible to close the current PR?

[echoix]

Oops, I think you committed your IDE support files

My goodness... There is so much mess I am making. Is it OK or possible to close the current PR?

I pushed a commit reverting the .idea folder, and applied the black formatting that made the workflow fail yesterday. By doing it that way instead of rebasing, it wouldn't cause too much conflicts on your side. I heard your call for help, and it wasn't a case of despair.

You're on the right track though! It takes a little time to learn our way through using git. But it doesn't take long, with practice.

[mshukuno]

Oops, I think you committed your IDE support files

My goodness... There is so much mess I am making. Is it OK or possible to close the current PR?

I pushed a commit reverting the .idea folder, and applied the black formatting that made the workflow fail yesterday. By doing it that way instead of rebasing, it wouldn't cause too much conflicts on your side. I heard your call for help, and it wasn't a case of despair.

You're on the right track though! It takes a little time to learn our way through using git. But it doesn't take long, with practice.

Thank you so much! I really appreciate it! CI failed notifications have been flooding my inbox and I've started to get nervous about pushing fixes. Thanks to you! I can now start fixing again in peace.

I have a question about doctest/code-block formatting. As I mentioned earlier, Ruff formats doctest/code-block and outputs (in doctest/code-block). Another reviewer recommended leaving the outputs as they are. So, reformatting code in doctest/code-block and keeping outputs as they are is the way to go?

[echoix]

I have a question about doctest/code-block formatting. As I mentioned earlier, Ruff formats doctest/code-block and outputs (in doctest/code-block). Another reviewer recommended leaving the outputs as they are. So, reformatting code in doctest/code-block and keeping outputs as they are is the way to go?

I'm not sure I know to what comment you're referring to. Reformatting the doctest/code-block (the inputs), should be fine, I agree. For the outputs, I see that it makes sense to try leaving them as is (to know if something breaks: regression testing), but I'm not convinced it would be breaking. The goal of doctests is to "copy and paste" the terminal output when you know it is correct, and the doctest module will make the test fail if even a space is wrong. https://docs.python.org/3/library/doctest.html#warnings Knowing that, I don't see how ruff would break that by reformatting the outputs, it seems breaking. In all cases, the answer would be to run these doc tests locally to make sure they still work before sending them in.

[petrasovaa]

I'm not sure I know to what comment you're referring to. Reformatting the doctest/code-block (the inputs), should be fine, I agree. For the outputs, I see that it makes sense to try leaving them as is (to know if something breaks: regression testing), but I'm not convinced it would be breaking. The goal of doctests is to "copy and paste" the terminal output when you know it is correct, and the doctest module will make the test fail if even a space is wrong. https://docs.python.org/3/library/doctest.html#warnings Knowing that, I don't see how ruff would break that by reformatting the outputs, it seems breaking. In all cases, the answer would be to run these doc tests locally to make sure they still work before sending them in.

The only way I was able to run them without failing was to split the output like that:

    >>> new_neighbors1.inputs.quantile = 0.5
    >>> new_neighbors1.get_bash()
    'r.neighbors input=mapD size=3 method=average \
weighting_function=none quantile=0.5 nprocs=1 memory=300 output=mapB'

which is not particularly pretty and so perhaps keeping them on one line is better. The other option is to use the ellipses to shorten the line, but I am not in favor of that.

[echoix]

Do you mind merging main into this PR and check that the changed Python files are also correct? It's almost the end!

[wenzeslaus]

Okay, this is merged. Thank you @mshukuno!

Couple PRs may need updates now given the extent of changes here (158 files). Check PR status at the bottom of the page of your PRs.