Allows bash modules to live anywhere #1
Conversation
…anywhere accessible by PYTHONPATH
| spec.script = script_path | ||
| return spec | ||
| # import is of the form: `from path.to.script import` | ||
| if len(name.split('.')) > 1: |
There was a problem hiding this comment.
I really like the idea here. The problem will be that the FS supports characters in path components which are not valid Python identifiers.
Would you be up to convert every character in name which is not a valid Python identifier to a _.
See https://docs.python.org/3/reference/lexical_analysis.html#identifiers for valid Python identifiers :)
There was a problem hiding this comment.
Indeed I agree this was probably the most fragile bit of the PR. I definitely didn't have time to come up with the proper solution for the scope of what I needed currently, but I certainly believe I can, and should, easily at least come up with a few rudimentary sanity checks before accepting random things :D
| # look for requested script in python path | ||
| for p in sys.path: | ||
| # convert script name to abs path | ||
| place = str(os.path.join(p, script_path)) |
There was a problem hiding this comment.
Let's go with pathlib here again :)
\
| place = str(os.path.join(p, script_path)) | |
| place = p / script_path |
There was a problem hiding this comment.
:) my non-native python ignorance
| for p in sys.path: | ||
| # convert script name to abs path | ||
| place = str(os.path.join(p, script_path)) | ||
| if os.path.exists(place): |
There was a problem hiding this comment.
| if os.path.exists(place): | |
| if place.exists(): |
Maybe we should check if it's really a script and not just a path which exists ?
There was a problem hiding this comment.
Agreed, I have the luxury in my environment of making assumptions, but this should definitely be robustified for general use.
Is there a good way you can think of beyond checks for execute privileges, '.sh' file extension verification, reading the first line of the file for a shebang? Those are the ideas off the top of my head before I dig in to investigate
|
The builds are currently failing - reason for that is the mixture of In any case: thanks for the contribution. I really appreciate it! 🎉 |
|
@timofurrer thanks for the feedback and pointing me to the test failures, my environment is in the >3.6 category which explains why I hadn't noticed :) I need to focus on getting some things worked out today so I likely won't get around to updating this until sometime in the next week or two when I have time again, but I do want to resolve the comments / suggestions! Thanks for building this in the first place, it is an elegant solution to the problem I've been wanting to solve for some time now - but I'm not a native pythoner so solutions like this didn't occur to me! |
|
No worries, keep me posted about the updates. If you need any help, don't hesitate to just ping me 🎉 |
I really liked the approach here, but I wanted to be able to import a shell script from a remote directory by including that location in
PYTHONPATHor withsys.path.append.I ran the tests on my machine and they appear to be good to go. I did not add any tests for remote bash execution because it would require a bit more setup.
Also note that the implementation isn't perfect and there are likely some edge cases that I haven't handled, but as it is, it fulfills my needs and if you'd like to take the functionality then feel free :)