-
Notifications
You must be signed in to change notification settings - Fork 1
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
enh: simplify nipype etelemetry ping handling #3
Conversation
wait, I think we don't need this much, really. |
6a9c8d1
to
dfb10ff
Compare
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.
I like the minimal approach, but one concern:
@@ -33,6 +33,7 @@ install_requires = | |||
psutil >= 5.4 | |||
pybids >= 0.10.2 | |||
pyyaml | |||
requests |
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.
at this point, why not just add etelemetry
directly and leverage the check there:
etelemetry.get_project("nipy/nipype")
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.
I thought of using _etrequest
directly, but we really don't want to do any kind of error handling. After going through the code it seemed more lightweight
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.
You bring up a good point... it would be useful to have an exception-free version in etelemetry
.
4ebe491
to
74d1920
Compare
74d1920
to
a8b5d03
Compare
Since we are setting pretty up-to-date nipype version restrictions on
setup.cfg
, the version check and logging it are unnecessary and confusing.But, we still want to record nipype users, so let's just ping manually.