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

Feature: User Macros #147

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

john-craig
Copy link

Implement User Defined Macros as per #125

@najohnsn najohnsn added the enhancement New feature or request label Oct 4, 2024
Copy link
Member

@najohnsn najohnsn left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! This looks really good! I still wanted to think about some things. In the meantime, I have some comments for you to consider.

tnz/zti.py Outdated
@@ -1838,6 +1846,45 @@ def help_plugin(entry=entry):

self.plugins = " ".join(plugins)

def __register_macros(self):
Copy link
Member

Choose a reason for hiding this comment

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

I believe the private methods are in alphabetical order. Please move to maintain alphabetical order.

tnz/zti.py Outdated
@@ -29,6 +29,7 @@
ZTI_AUTOSIZE
ZTI_SECLEVEL (see tnz.py)
ZTI_TITLE
ZTI_MACROS_DIR (~/.zti-macros.d is default)
Copy link
Member

Choose a reason for hiding this comment

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

I haven't tried yet... but does this cause some strangeness on windows because it's longer than 8 characters? I wonder if a shorter name might make things more pleasant.

tnz/zti.py Outdated
if hasattr(macro_module, macro_name):
# Create a new bound method for the `Zti` class for this
# function
setattr(Zti, f"do_{macro_name}",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would make more sense if the function name in the .py file were do_{macro_name}... that way the name would be the same. And maybe make an optional help_{macro_name} so that help can be provided - for a "help {macro_name}" command.

tnz/zti.py Outdated
import sys
import types

if not os.path.isdir(self.macros_dir):
Copy link
Member

Choose a reason for hiding this comment

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

The name of the default macro dir is very zti specific. I would have a hard time believing the name would be used for something else. So, some error messages might be in order if things don't look right. Could help the user if they do things wrong accidentally.

Signed-off-by: John Craig <[email protected]>
# function
setattr(Zti, f"do_{macro_name}",
types.MethodType(
getattr(macro_module, f"do_{macro_name}"),
Copy link
Member

Choose a reason for hiding this comment

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

You may have noticed that many of the do_<command> methods call self.__bg_wait_end(). That is because a background thread runs while the user is at the prompt and that call shuts down that background thread so that the session(s) can be used in a thread-safe manner. That is something that is definitely needed here. Without it. both the thread running the do_<command> method and the background thread could be using tnz methods that are not thread-safe. I suggest you define a method that wraps the macro method in order to accomplish this. See do_plugin() as an example.

Copy link
Member

Choose a reason for hiding this comment

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

Another thing to model after do_plugin() ...

                    with ati.ati.new_program(share_sessions=True):
                        plugin(arg, **kwargs)

That helps ensure that the ati environment provided to the plugin is clean and that any variables set by the plugin do not leak into the current environment. Unless your use case requires non-shared/global ati variables as input to or output from the macro, I suggest being consistent with do_plugin().

if len(macro_file.split('.')) > 1:
continue

macro_name = macro_file.split('.')[0]
Copy link
Member

Choose a reason for hiding this comment

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

Since precmd() converts commands to lowercase, any command with a capital letter will not work. Along those lines... also consider ignoring/rejecting macros that have a space in the name.

@@ -115,6 +116,11 @@ def __init__(self, stdin=None, stdout=None):
if os.getenv("ZTI_AUTOSIZE"):
self.autosize = True

self.macros_dir = os.getenv("ZTI_MACROS_DIR")
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be no reason to make macros_dir a property of Zti. I suggest just doing the environment variable inspection in __register_macros(). If you would rather do the environment variable inspection here... then pass macros_dir into __register_macros().

@@ -115,6 +116,11 @@ def __init__(self, stdin=None, stdout=None):
if os.getenv("ZTI_AUTOSIZE"):
self.autosize = True

self.macros_dir = os.getenv("ZTI_MACROS_DIR")
if self.macros_dir is None or not os.path.isdir(
self.macros_dir):
Copy link
Member

Choose a reason for hiding this comment

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

Don't hide the error if the user has ZTI_MACROS_DIR=not_a_dir. I suggest just removing that part of the check.

@@ -0,0 +1,31 @@
from tnz.ati import ati

def logon(zti, arg):
Copy link
Member

Choose a reason for hiding this comment

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

now need to rename to do_logon

# disconnect from host
ati.drop("SESSION")

def logon_helper():
Copy link
Member

Choose a reason for hiding this comment

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

unused function?

ati.set("ONERROR", "1")
ati.set("DISPLAY", "HOST")
ati.set("SESSION_HOST", "mvs1")
ati.set("SESSION", "A")
Copy link
Member

Choose a reason for hiding this comment

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

missing newline

macro_name = macro_file.split('.')[0]

# Ignore macros which already exist
if f"do_{macro_name}" in self.get_names():
Copy link
Member

Choose a reason for hiding this comment

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

may want to consider a warning message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants