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

one source of truth for ~/.sami2py #74

Merged
merged 6 commits into from
Jul 8, 2019
Merged

Conversation

scivision
Copy link
Contributor

In regulatory / compliance bound environments, messing with sys.prefix is not allowed. Many packages (Python and otherwise) install under ~/.my_pkg_name instead of sys.prefix in any case. This PR follows that convention.

Also, makes one source of truth (well almost,setup.py and __init__.py) for this directory.
Also a bit of PEP8 / namespace cleanup.

We have been working off a fork that combines this PR + #72 + #73 to make sami2py usable in general.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@JonathonMSmith
Copy link
Collaborator

JonathonMSmith commented Jul 3, 2019

Maybe this PR isn't the place for this discussion, but with these changes, if there are multiple installations of sami2py in several VEnvs then they will all share the ~/.sami2py contents. The use of sys.prefix was added so that virtual machines would always have their own ".sami2py". Clearly, this breaks for regulatory / compliance bound environments so it must be removed. Perhaps there's another solution to this or it's not considered an issue at all and the contents should just be changed manually when switching between environments.

@scivision
Copy link
Contributor Author

scivision commented Jul 5, 2019

OK I wondered about that for venv as a motivator. Possibilities include:

  • make this .sami2py location user-selectable at install (I can't even install sami2py with sys.prefix/.sami2py)
  • make the .sami2py location under ~/.sami2py/<venv name> where perhaps venv_name = os.path.split(sys.prefix)[-1]

Re: the second options, this page discusses detecting if one is inside a virtualenv.

The second option is nice because it retains the automatic configuration, and it appears initially via the stackoverflow answer, the ~/.sami2py/venv_name may be possible

@jklenzing jklenzing added this to the 0.2.0 Release milestone Jul 5, 2019
@jklenzing
Copy link
Member

I think I've sorted out the merge conflicts and kept the intent of this PR. Let me know if something looks off.

As for the venv issues, I like the second option. Not sure if we should try to address that here or in a separate PR. @JonathonMSmith?

@scivision
Copy link
Contributor Author

scivision commented Jul 5, 2019

208b912 fixes a couple typos from the rebase on develop--it matches the scivision:next branch we have been using

@JonathonMSmith
Copy link
Collaborator

I'm happy with leaving the venv for another issue/PR

@scivision
Copy link
Contributor Author

Yea in essence this allows anyone that doesn't have sudo/admin access, or even the ability to modify the directories under the Python install, to install sami2py.

@jklenzing
Copy link
Member

jklenzing commented Jul 8, 2019

OK. Writing the venv stuff up as a new issue.

@jklenzing jklenzing merged commit 3f037ad into sami2py:develop Jul 8, 2019
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.

None yet

3 participants