-
Notifications
You must be signed in to change notification settings - Fork 74
Correct lint issues exposed with update to pylint-3 #1652
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
Conversation
@@ -133,7 +133,7 @@ def test_reflection_using_prepare(num_ones, eps, global_phase): | |||
prepared_state = result.final_state_vector.reshape(2 ** len(selection), -1).sum(axis=1) | |||
if np.sign(global_phase) == 1: | |||
signs = '-' * num_ones + '+' * (9 - num_ones) | |||
elif np.sign(global_phase) == -1: | |||
else: # np.sign(global_phase) == -1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we keep this as an elif and add an else clause that raises a ValueError or AssertionError?
Can you revert the changes to remove the different types. The tooling should support us writing good code; we shouldn't let the tooling dictate worse patterns. Unless there's something I'm missing about the original pattern actually being wrong (??) |
This reverts commit 7fbc8f3.
@mpharrigan Pushed the reversion. My thought process is that the current state isn't good code since we are using abstraction purely for |
The distinction shows up in calls to the constructor, with the unique class names helping to make the code more self-documenting. Should we keep |
I can definitely confirm that last part, that a skip would be necessary to use these classes. Parsing through the code I do think it's all false positives. I don't believe any abstract classes are getting instantiated. It would be nice to be able to keep the warning for real cases, but I'm not sure it's worth it with all the false positives. |
Not sure why anything is failing out now since all that has changed between now and the last succeeding CI was comments after the reversion...? Oh, I see the notebooks need updating. I'll wait until there's agreement on what to do with |
Yes, I think we should keep If we instantiate an abstract class, it will get caught at runtime (e.g. during a unit test) |
There's also a known flaky test at the moment #1657 , which is the |
This reverts commit e238c90.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for getting these three checks fixed and enabled; and investigating the final one to be false-positives
Fixes #1440
Re-enables the following pylint issues which were disabled when bumping to pylint-3.