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.

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

Worked on fixing issue W0612 (unused-variable) in "common" #1790

Merged
merged 6 commits into from
Aug 11, 2024

Conversation

dnerever
Copy link
Contributor

@dnerever dnerever commented Jul 11, 2024

Fixed the occurrence of unused variables through multiple files within /common

Related to #1755

@buhtz
Copy link
Member

buhtz commented Jul 11, 2024

Hello Keith,
Thank you for that PR. You see it can get complicated. I added some comments. Let me know how you want to proceed.

Because of its complexity and possible impact I schedule that PR until the upcoming release (see milestone).

@buhtz buhtz added this to the 2nd release from now milestone Jul 11, 2024
@buhtz buhtz added Discussion decision or consensus needed PR: Modifications requested Maintenance team requested modifications and waiting for their implementation labels Jul 11, 2024
@buhtz buhtz marked this pull request as draft July 11, 2024 07:23
@dnerever
Copy link
Contributor Author

Hey Christian,
Can you direct me to where you added your comments.

I do see how it can get complicated so delaying it makes sense. Any suggestions for ensuring my future PRs avoid introducing complexity? I'm going now to take a look at the other opens issues in the repo.

Thanks again for all the help!

@buhtz
Copy link
Member

buhtz commented Jul 12, 2024

You are not the one making it complicated. The old and smelly Back In Time code does. I just tried to pointing out that even simple modifications can become complicated in old code base like Back In Time has.

About the comments. I added 4 comments directly to the diff-like output. This means a comment is connected directly to a line in the code. I marked them with yellow circles in this screenshot. Just scroll up.

image

@dnerever dnerever marked this pull request as ready for review July 13, 2024 05:51
common/config.py Outdated Show resolved Hide resolved
common/schedule.py Show resolved Hide resolved
common/sshtools.py Outdated Show resolved Hide resolved
common/sshtools.py Show resolved Hide resolved
@buhtz buhtz added PR: Merge after creative-break Merge after creative-break (min. 1 week) and removed Discussion decision or consensus needed PR: Modifications requested Maintenance team requested modifications and waiting for their implementation labels Jul 13, 2024
@buhtz buhtz changed the title Worked on fixing issue W0612, unused-variable Worked on fixing issue W0612 (unused-variable) in "common" Aug 6, 2024
@buhtz
Copy link
Member

buhtz commented Aug 6, 2024

To prevent you from extra work and wasting time, I just want to mention that someone else will work on the same rule but in "qt" folder.

#1755 (comment)

EDIT: By the way I realized that you did not activate the W0612 rule in common/test/test_lint.py in this PR. Is there are reason? If all W0612 errors in "common" are fixed you can activate it.

Copy link
Member

@buhtz buhtz left a comment

Choose a reason for hiding this comment

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

Please see my last comment.

@buhtz buhtz added PR: Modifications requested Maintenance team requested modifications and waiting for their implementation and removed PR: Merge after creative-break Merge after creative-break (min. 1 week) labels Aug 6, 2024
@buhtz
Copy link
Member

buhtz commented Aug 11, 2024

I see why you didn't enable W0612 by default. There are to many errors.

@buhtz buhtz removed the PR: Modifications requested Maintenance team requested modifications and waiting for their implementation label Aug 11, 2024
@buhtz buhtz added the PR: Merge after creative-break Merge after creative-break (min. 1 week) label Aug 11, 2024
@buhtz buhtz merged commit 0ba4396 into bit-team:dev Aug 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Merge after creative-break Merge after creative-break (min. 1 week)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants