-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fastpath: Add ooni_run_link_id support #768
Conversation
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.
🐳
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.
lgtm
36ea57e
to
ef88df5
Compare
2962bb8
to
0cd0af3
Compare
fastpath/fastpath/core.py
Outdated
ooni_run_link_id_str = g_or(annot, "ooni_run_link_id", "") | ||
if ooni_run_link_id_str: | ||
try: | ||
ooni_run_link_id = int(ooni_run_link_id_str) | ||
except ValueError: | ||
ooni_run_link_id = None | ||
else: | ||
ooni_run_link_id = None |
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.
Isn't it a bit more clear to rewrite this as:
ooni_run_link_id_str = g_or(annot, "ooni_run_link_id", "") | |
if ooni_run_link_id_str: | |
try: | |
ooni_run_link_id = int(ooni_run_link_id_str) | |
except ValueError: | |
ooni_run_link_id = None | |
else: | |
ooni_run_link_id = None | |
ooni_run_link_id_str = g_or(annot, "ooni_run_link_id", None) | |
try: | |
ooni_run_link_id = int(ooni_run_link_id_str) | |
except (TypeError, ValueError): | |
ooni_run_link_id = None |
?
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.
It would be shorter but it would raise and catch an exception around int() even for the expected value ""
.
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.
It would not raise the exception, since int("")
throws a ValueError
which is caught by the handler.
It's equivalent to the previous code.
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.
Made small suggestion to the diff. Otherwise LGTM
0.84~pr768-221 deployed on -test |
0.84~pr768-221 deployed on FSN |
Implements: #701
Specs: ooni/spec@220aaea