Removing broad-exception-raised lint rule and updates #12356
Conversation
joesho112358
requested review from
nonhermitian,
a team,
alexanderivrii and
ShellyGarion
as code owners
qiskit-bot
added
the
Community PR
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 9032720722Details
💛 - Coveralls |
There was a problem hiding this comment.
Thanks for looking at this - this is definitely a good lint to get enabled. I left a couple of comments about avoiding RuntimeError - we usually try and avoid that one as much as possible because there's generally some tighter bound to apply. It's all a little bit subjective of course, but we (at least I!) generally try to keep RuntimeError for exceptions in defensive programming bits amongst ourselves, like if an internal private invariant has somehow become violated.
(Fwiw, it matters rather less what we use in tests, since if we're ever raising an Exception in tests, it's usually just to indicate a test failure or something.)
# Conflicts: # pyproject.toml
seems fair to me! updates are in for another look |
| def test_mul_by_array(self): | ||
| """Quaternions cannot be multiplied with an array.""" | ||
| other_array = np.array([0.1, 0.2, 0.3, 0.4]) | ||
| self.assertRaises(Exception, self.quat_unnormalized.__mul__, other_array) | ||
| self.assertEqual(NotImplemented, self.quat_unnormalized.__mul__(other_array)) | ||
|
|
||
| def test_mul_by_scalar(self): | ||
| """Quaternions cannot be multiplied with a scalar.""" | ||
| other_scalar = 0.123456789 | ||
| self.assertRaises(Exception, self.quat_unnormalized.__mul__, other_scalar) | ||
| self.assertEqual(NotImplemented, self.quat_unnormalized.__mul__(other_scalar)) |
There was a problem hiding this comment.
This is pre-existing, stemming from the previous implementation of these magic methods being incorrect, but in general we don't want to call __mul__ directly - NotImplemented is just a sentinel value that's properly interpreted by Python. The normal forms of these tests would be
with self.assertRaises(TypeError):
_ = self.quat_unnormalized * other_arrayand similar for the other one.
This actually means that Python will also try the __rmul__ methods on other_array and other_scalar as well before raising the TypeError, in case either of those classes is something that can explicitly handle multiplication by Quaternion, though in this case, both will fail. The Numpy array test will actually start to succeed (Numpy will attempt to broadcast the multiplication), but then it'll fail the internal scalar multiplication.
There was a problem hiding this comment.
i see. yeah, i was thinking this was more of a unit test vs integration test. updates on the way!
# Conflicts: # pyproject.toml
jakelishman
added
type: qa
* Removing broad-exception-raised lint rule and updates * updating assertion types based on review * updating tests based on review
Summary
#9614 removing
broad-exception-raisedDetails and comments
Updates for removing the
broad-exception-raisedrule