Reduce flake8 max-line-length. #3133
Conversation
|
@benclifford Is there anything I should take note of next time? |
.flake8
Outdated
| max-line-length = 152 | ||
| max-line-length = 153 |
There was a problem hiding this comment.
Hmm; this number has gotten bigger, not smaller. Per the ticket, I'm positive you correctly reduced the value initially ... so what happened and when? What do you need to do now?
There was a problem hiding this comment.
It was modified from 154 to 153, then i pushed to the repo and initiated a pull request but got a conflict. So I think the conflict was triggered by duplicates line of code along with the conflict markers, so i just deleted one line and the markers because you need to decide which version of the max-line-length setting you want to keep. Since both versions are adjacent and come from the same branch (master), you can simply choose one of them.
There was a problem hiding this comment.
*nod; I understand. I'm asking you to think critically. What is the point of the ticket? And, more precisely, what is the specific instruction?
When that is clear, what needs to happen here? I understand the steps you've taken, but now what would otherwise be merged is not accurate to the ticket.
There was a problem hiding this comment.
Making the line length smaller, then I will test and validate. I will update my pull request. What do you think please?
There was a problem hiding this comment.
Hi @khk-globus,
I wanted to inform you that I have committed and pushed the latest changes to my repository.
Please review the updates at your convenience, and please let me know if you have any feedback or questions.
There was a problem hiding this comment.
Thank you; FWIW ("for what it's worth"), you don't need to explicitly tell me this type of detail. When one enters a PR conversation (e.g., making the PR, commenting on the PR), GitHub will email them thereafter.
There was a problem hiding this comment.
Please review the updates at your convenience, and please let me know if you have any feedback or questions.
Now the hunk to change the line-length is missing entirely. Again, what is the point of the ticket? What is the specific task for the PR?
There was a problem hiding this comment.
@khk-globus Please what do you suggest I do.
There was a problem hiding this comment.
*nod; I understand. I'm asking you to think critically. What is the point of the ticket? And, more precisely, what is the specific instruction?
@khk-globus Please what do you suggest I do.
This is the "think critically" part. I know what I might do, but that's not important. This is your pull request, not mine. A couple of thoughts come to mind:
- What does GitHub show will be changed if this PR is merged? (Hint: "Files changed" tab)
- What does issue Outreachy: getting familiar with the development environment #3105 state to do?
Do you see a discrepancy?
There was a problem hiding this comment.
Thank you @khk-globus, This shows that my PR isnt Aligned with the goals of the Issue #3105
There was a problem hiding this comment.
In general, PRs should remain focused on one logical change.[1] The title of this PR is "Reduce flake8 max-line-length" — a good, focused change.
As of the recent merge, this PR now has some unrelated changes, currently:
changelog.rstdesign.rstindex.rst
As I was typing index.rst, I see that the ticket closed, so I suspect you saw that detail. I'm guessing at what happened, so let me draw your attention to GitHub's text near the top of the ticket:
Olotuah wants to merge 7 commits into Parsl:master from Olotuah:master
In slightly different language, that states that if this PR is merged, Parsl's master will take in the changes from Olotuah's master branch.
Does that give you a hint as to how to approach having multiple PRs open at the same time? Or how to update your master branch without affecting your open PRs?
[1] For now, I simply make this statement, but I may ask in a follow up why PRs should remain focused on a single logical change.
|
@khk-globus It was a mistake from my end, I saved the message as "Add Historical Documents section and update index" but my branch got merge and i saw the commit message had change , i immediately closed it when i noticed. I have to Regularly fetch the changes from the upstream repo to my branch so its updated and can I always create a new branch for each PRs? |
Description
This pull request reduces the Flake8 max-line-length in the
process_worker_pool.pyfile to improve code readability and maintainability. It addresses the Flake8 error reported on line 872 ofprocess_worker_pool.pyby splitting a long line of code into multiple shorter lines.Changed Behaviour
After this PR, the maximum line length in the
process_worker_pool.pyfile will be reduced, and the code will adhere to the Flake8 style guide, which recommends a maximum line length of 79 characters for improved readability.Fixes
Fixes #3126
Type of change
Choose which options apply, and delete the ones which do not apply.