NRLMMD-GEOIPS / geoips

Main Geolocated Information Processing System code base with basic functionality enabled.
https://nrlmmd-geoips.github.io/geoips/
Other
13 stars 10 forks source link

Address how we want to handle legacy procflow arguments with the CLI #638

Closed evrose54 closed 1 week ago

evrose54 commented 2 weeks ago

Requested Update

Description

With the changes made in #465, we've refactored how we'll call our processing workflows (procflows) for the foreseeable future. You can still run a procflow via run_procflow or data_fusion_procflow, however we recommend that you change over to geoips run <procflow_name> instead. Since we now pipe the execution of a procflow through the CLI, we should discuss how we'd like to handle the legacy procflows' arguments. Currently, run_procflow's arguments are stored in geoips.commandline.args:add_args and the help messages are formatted largely as multiline strings, which can cause some formatting issues. Since all of these arguments can be applied to geoips run <procflow_name>, we should discuss whether or not we'd like to keep these arguments as they are now or if we should move them to the cmd_instructions.yaml file.

Personally I think we should leave the argument help strings where they are currently, as this seems like a large and unfruitful refactor. However, I also see the benefit of all commandline arguments being located in the same place. I think this needs to be a group decision.

Background and Motivation

This stems from this comment on PR #465.

Code to demonstrate issue

Checklist for Completion

jsolbrig commented 2 weeks ago

I think this issue also relates to how we process the docstrings prior to outputting them and how we write the docstrings. Currently we strip out any blank lines prior to outputting the docstrings. This makes the docstrings more difficult to read. We should consider how we want to handle this.

Personally, I think we should print docstrings exactly as specified except for leading spaces.

jsolbrig commented 1 week ago

This was handled in #465. Closing. Will discuss if current solution is appropriate when going over the CLI next week.