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

Move scripts out of lib/tool_shed. #2093

Merged
merged 4 commits into from
Apr 8, 2016
Merged

Conversation

davebx
Copy link
Contributor

@davebx davebx commented Apr 5, 2016

No description provided.

@davebx davebx added kind/refactoring cleanup or refactoring of existing code, no functional changes area/cleanup minor area/toolshed labels Apr 5, 2016
@jmchilton
Copy link
Member

The minor tag means it changes made by devteam members to features released this cycle - it doesn't apply here.

@jmchilton jmchilton removed the minor label Apr 5, 2016
@@ -4,7 +4,7 @@
import urllib
import urllib2

sys.path.insert(1, os.path.join( os.path.dirname( __file__ ), os.pardir, os.pardir, os.pardir ) )
sys.path.insert( 1, 'lib' )
Copy link
Member

Choose a reason for hiding this comment

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

This should be

sys.path.insert(1, os.path.join(os.path.dirname( __file__ ), os.pardir, os.pardir, os.pardir, 'lib'))

@nsoranzo
Copy link
Member

nsoranzo commented Apr 5, 2016

sys.path.insert() should be updated also in scripts/tool_shed/bootstrap_tool_shed/create_user_with_api_key.py and the other Python files following it in the list of moved files.

@davebx
Copy link
Contributor Author

davebx commented Apr 5, 2016

@jmchilton Where is this documented? I should probably bookmark that page.

@jmchilton
Copy link
Member

👍 It wasn't really obvious to me at first that scripts belong in scripts/ and not lib/ but I think these do since they mess with sys.path. I think as a design goal - nothing in lib/ should be messing with sys.path - all modules in lib should be importable, side-effect free, without errors if non-optional dependencies are available.

@jmchilton jmchilton merged commit 2744d55 into galaxyproject:dev Apr 8, 2016
@davebx
Copy link
Contributor Author

davebx commented Apr 8, 2016

@jmchilton In addition to the above, with which I agree, I don't see lib/ being appropriate for scripts run from the command line, nor scripts/ being appropriate for modules loaded into galaxy, the toolshed, or whichever app one is dealing with.

@nsoranzo
Copy link
Member

nsoranzo commented Apr 8, 2016

@davebx sys.path.insert() still need to be updated in many Python files moved in scripts/tool_shed/:
scripts/tool_shed/check_filesystem_for_empty_tool_dependency_installation_paths.py
scripts/tool_shed/check_repositories_for_functional_tests.py
scripts/tool_shed/check_s3_for_empty_tool_dependency_installation_paths.py
scripts/tool_shed/check_tool_dependency_definition_repositories.py
scripts/tool_shed/deprecate_repositories_without_metadata.py
scripts/tool_shed/show_tool_dependency_installation_dir_contents.py

@galaxybot
Copy link
Contributor

This PR was merged without a milestone attached.

@martenson
Copy link
Member

Is this consistent now? @nsoranzo @davebx

this seems to have caused #2121

@martenson martenson added this to the 16.07 milestone Apr 11, 2016
@nsoranzo nsoranzo modified the milestones: 16.04, 16.07 Apr 11, 2016
@martenson martenson mentioned this pull request Apr 11, 2016
nsoranzo added a commit to nsoranzo/galaxy that referenced this pull request Apr 11, 2016
nsoranzo added a commit to nsoranzo/galaxy that referenced this pull request Apr 11, 2016
jmchilton added a commit that referenced this pull request Apr 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/toolshed kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants