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

Platform agnostic CLI #63

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

Conversation

AT0myks
Copy link
Contributor

@AT0myks AT0myks commented Nov 28, 2022

Here's the new PR, if I didn't make any mistakes the scripts should behave exactly the same as before.

I didn't update the readme to reflect the changes because I'm not sure how you would like it to be edited.

Create a file for each script and move their remaining code there.
Add the base parser as a parent of each moved parser.
Adapt the scripts to the new layout.
@jrspruitt
Copy link
Contributor

Hi AT0myks

Thank you, I'm a little busy at the moment, and this will take some time to review and change my test set up, but I will get to it.

-Jason

@jrspruitt
Copy link
Contributor

Hi AT0myks,

Just an update, I've been working on this. I took the idea a little further and made a UIParser class, where the most common cli arguments could go, along with handling the checks and logic for the variables they populate. I got a couple of the scripts working, ubireader_extract_files and ubireader_extract_images. It gets rid of a lot of duplicate code, but does require function calls to add args. I think you could still use this from a script. Any thoughts?

platform-agnostic-cli

-Jason

@AT0myks
Copy link
Contributor Author

AT0myks commented Dec 5, 2022

That looks pretty good to me. Removing the duplicate code at the beginning of each function was on my list.
I don't see any obvious problem when using this from a script.

Here are my thoughts:

I think usage and description could be UIParser arguments:

class UIParser(object):
    def __init__(self, usage=None, description=None):
        self._p = argparse.ArgumentParser(add_help=False, usage=usage, description=description)

Use property decorators:

def set_usage(self, usage):
    self._p.usage = usage
def get_usage(self):
    return self._p.usage
usage = property(get_usage, set_usage)

becomes

@property
def usage(self):
    return self._p.usage
@usage.setter
def usage(self, usage):
    self._p.usage = usage

In UIParser there could be a method like add_common_args which adds the 10 common arguments so that, for example in extract_files, you can replace

ui.arg_keep_permissions()
ui.arg_log()
ui.arg_verbose_log()
ui.arg_peb_size()
ui.arg_start_offset()
ui.arg_end_offset()
ui.arg_guess_offset()
ui.arg_warn_only_block_read_errors()
ui.arg_ignore_block_header_errors()
ui.arg_u_boot_fix()
ui.arg_output_dir()
ui.arg_filepath()

by

ui.add_common_args()
ui.arg_keep_permissions()
ui.arg_output_dir()

It also helps identifying the unique arguments of each parser and makes it a bit easier to add common arguments later.

filepath being a required positional argument, I think it should be an argument of parse_args:

def parse_args(self, filepath, *args, **kwargs):

and then also of all parser functions

def extract_files(filepath, *args, **kwargs):

This way in another script you can call the functions with the default arguments with extract_files("path") instead of having to give it as a keyword argument like extract_files(filepath="path").

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.

2 participants