Skip to content
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.

Sign up for GitHub

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

Reduce flake8 max-line-length. #3133

Closed
wants to merge 7 commits into from
Closed

Reduce flake8 max-line-length. #3133

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
723db93
Reduce flake8 max-line-length.
Olotuah Mar 5, 2024
2d6561b
Merge branch 'master' into master
Olotuah Mar 5, 2024
9e49062
Reduce flake8 max-line-length.
Olotuah Mar 5, 2024
910b470
Reduce flake8 max-line-length
Olotuah Mar 5, 2024
c31b412
Reduce flake8 max-line-length
Olotuah Mar 5, 2024
a7cc871
Add Historical Documents section and update index
Olotuah Mar 6, 2024
5f79977
Merge branch 'master' into master
Olotuah Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .flake8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
# W504: line break after binary operator
# (Raised by flake8 even when it is followed)
ignore = E126, E402, E129, W504
max-line-length = 152
max-line-length = 153
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the line length smaller, then I will test and validate. I will update my pull request. What do you think please?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khk-globus Please what do you suggest I do.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*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:

Do you see a discrepancy?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @khk-globus, This shows that my PR isnt Aligned with the goals of the Issue #3105

exclude = test_import_fail.py,
parsl/executors/workqueue/parsl_coprocess.py
# E741 disallows ambiguous single letter names which look like numbers
Expand Down
7 changes: 3 additions & 4 deletions parsl/executors/high_throughput/process_worker_pool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,10 +869,9 @@ def strategyorlist(s: str):
block_id=args.block_id,
cores_per_worker=float(args.cores_per_worker),
mem_per_worker=None if args.mem_per_worker == 'None' else float(args.mem_per_worker),
max_workers_per_node=(
args.max_workers_per_node if args.max_workers_per_node == float('inf')
else int(args.max_workers_per_node)
),

max_workers_per_node=(args.max_workers_per_node if args.max_workers_per_node == float('inf')
else int(args.max_workers_per_node)),
prefetch_capacity=int(args.prefetch_capacity),
heartbeat_threshold=int(args.hb_threshold),
heartbeat_period=int(args.hb_period),
Expand Down
Loading