Skip to content
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

Clean up import of isi_sdk_7_2 and pytest #17

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Clean up import of isi_sdk_7_2 and pytest #17

wants to merge 10 commits into from

Conversation

cbrainerd
Copy link

@cbrainerd cbrainerd commented Dec 5, 2016

Summary

This PR cleans up some issues lingering with the rename of isi_sdk to isi_sdk_7_2 and the the move to PyPI distribution along with an import of pytest that was occurring when hexaparse was run from CLI.

  • No need for an explicit installation step for isi_sdk_7_2 since it is included in requirements.txt
  • Remove the misleading print of the ImportError when falling back to isi_sdk failed, as this message implied that the failure was due to missing isi_sdk whereas isi_sdk_7_2 is preferred.
  • Moved import of pytest in hexaparse so that it only occurs when unit tests are run.
  • Amend README-dev to include npm install of phantomjs-builtin, which is needed for casperjs

Testing done

  • Python unittests pass
  • JS unittests pass
  • Created distributable zip and performed install and built browser in a virtualenv

@cbrainerd cbrainerd changed the title Clean up import of isi_sdk_7_2 Clean up import of isi_sdk_7_2 and pytest Dec 5, 2016
@cbrainerd
Copy link
Author

Fixes #16

@@ -5,10 +5,10 @@
try:
# Try the original SDK library name
import isi_sdk
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this should be:

import isi_sdk_7_2

or

import isi_sdk_7_2 as isi_sdk

Copy link
Author

Choose a reason for hiding this comment

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

The intent here is to allow functioning with either isi_sdk_7_2 or the original and now deprecated isi_sdk library.

So it first tries importing isi_sdk_7_2, if that fails then it tries for the deprecated SDK library.

@@ -14,10 +14,10 @@
try:
# Try the original SDK library name
import isi_sdk
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here.

Copy link
Author

Choose a reason for hiding this comment

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

Same situation here with allowing either isi_sdk or isi_sdk_7_2

Copy link
Contributor

@apecoraro apecoraro left a comment

Choose a reason for hiding this comment

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

Looks good to me, although I'm not familiar with most of the code. I did pull down this branch and test that it works on 7.2, 8.0.1, and 8.1 clusters.

@@ -1,3 +1,4 @@
future>=0.15.2
jinja2>=2.8
urllib3 >= 1.15
urllib3>=1.15
isi_sdk_7_2>=0.1.2
Copy link

Choose a reason for hiding this comment

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

0.2.x contains backwards-incompatible changes (see #19).

Suggested change
isi_sdk_7_2>=0.1.2
isi_sdk_7_2~=0.1.2

The compatible release operator should cause the latest compatible isi_sdk_7_2 to be installed (currently, 0.1.11).

@@ -5,3 +5,4 @@ mock>=2.0.0
jinja2>=2.8
pylint>=1.5.5
urllib3 >= 1.15
isi_sdk_7_2>=0.1.2
Copy link

Choose a reason for hiding this comment

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

Consider referencing requirements.txt instead of duplicating it here.

Suggested change
isi_sdk_7_2>=0.1.2
-r requirements.txt

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.

3 participants