Skip to content

Conversation

@VDubber
Copy link

@VDubber VDubber commented Jun 1, 2023

Please test this locally since no tests are run on GitHub!!

@VDubber VDubber requested a review from a team as a code owner June 1, 2023 20:22
raise JupiterOneApiError('{}:{}'.format(response.status_code, content.get('error')))
try:
content = json.loads(response._content)
raise JupiterOneApiError('{}: {}'.format(response.status_code, content.get('error') or 'Unknown Error'))

Choose a reason for hiding this comment

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

I'm a bit rusty with Python, but I think the or operator doesn't handle nullish coalescing like we'd expect in a TypeScript world. I'm assuming that's why we need the additional try/catch here to catch the ValueError?

We could do some kind of if statement optionally, but I think this solution is fine too.

Copy link
Author

@VDubber VDubber Jun 1, 2023

Choose a reason for hiding this comment

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

I based this logic off of this post:
https://stackoverflow.com/questions/4978738/is-there-a-python-equivalent-of-the-c-sharp-null-coalescing-operator
And I was noticing that content.get was returning None which should be handled by or

Choose a reason for hiding this comment

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

Nice! So is the additional try/catch to handle something else, or more of a belt and suspenders kind of thing?

Copy link
Author

Choose a reason for hiding this comment

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

Def belt and suspenders. The unknown handler :)

@VDubber VDubber merged commit 9155762 into main Jun 2, 2023
@VDubber VDubber deleted the INT-8443-handle-401 branch June 8, 2023 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants