Skip to content
This repository was archived by the owner on Mar 7, 2025. It is now read-only.

Allows bash modules to live anywhere #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allows bash modules to live anywhere #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions shellfuncs/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,25 @@ class ShellScriptFinder:
"""
@classmethod
def find_spec(cls, name, path=None, target=None):
script_path = Path('{0}.sh'.format(name.replace('.', '/')))
if not script_path.exists(): # no such script found.
return None

# TODO: check relative imports and directory walking
loader = ShellScriptLoader()
spec = importlib.util.spec_from_loader(name, loader)
spec.script = script_path
return spec
# import is of the form: `from path.to.script import`
if len(name.split('.')) > 1:
Copy link
Owner

Choose a reason for hiding this comment

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

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 :)

Copy link
Author

Choose a reason for hiding this comment

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

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

# need to convert any . to / for path finding
script_path = Path('{0}.sh'.format(name.replace('.', '/')))
else:
script_path = name + '.sh'

# 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))
Copy link
Owner

Choose a reason for hiding this comment

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

Let's go with pathlib here again :)
\

Suggested change
place = str(os.path.join(p, script_path))
place = p / script_path

Copy link
Author

Choose a reason for hiding this comment

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

:) my non-native python ignorance

if os.path.exists(place):
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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 ?

Copy link
Author

Choose a reason for hiding this comment

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

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

# if we found the requested script, load it
loader = ShellScriptLoader()
spec = importlib.util.spec_from_loader(name, loader)
spec.script = place
return spec

return None


class ShellScriptLoader:
Expand Down Expand Up @@ -105,7 +115,12 @@ def execute_func(self, name, *args, shell=None, env=None, stdin=None, timeout=No
"""
Execute the shell function with the given name.
"""
cmdline = '. ./{script} && {func} {args}'.format(
# assume no leading slash means it isnt abspath
# therefore, we should prepend './' for relative execution
if self.script[0] != '/':
self.script = './' + self.script

cmdline = '. {script} && {func} {args}'.format(
script=str(self.script),
func=name, args=' '.join("'{0}'".format(x) for x in args))

Expand Down