This issue was created due to lingering comments on the #444 PR. Since that PR is a dependency of other PRs like #465 and #478 , we are going to wait to make those updates and finish them up in a separate PR once those have been merged. Since none of the comments address actual bugs, this is largely just a refactoring change and is good to wait until we merge the aforementioned PRs.
Checklist for Completion
Add to the following list for all comments that still need addressing.
[ ] Use consistent naming for pkg in CLI Unit tests
[ ] Address whether or not we want to use .extend(list) rather than += list. Personally I say we keep it as +=
[ ] Address if we want to refactor all_possible_subcommand_combinations for CLI Unit Tests (See #648)
[ ] Consider renaming all_possible_subcommand_combinations to something more accurate. Some include all cases; some are stochastic. (See #648)
[ ] Consider making separate geoips unit / unit-test command rather than geoips test unit-test (not important right now as this functionality has been completely commented out)
[ ] Move GeoipsExecutableCommand:_get_registry_by_interface_and_package to PluginRegistry class
[ ] Consider refactoring tabulate portions of GeoipsList to reduce some code duplication
[ ] Fix formatting of usage when a command is called incorrectly. For example, geoips list prints usage that is poorly formatted.
[ ] Consider refactoring GeoIPS Subcommand Classes into a different hierarchical organization. This will need ample discussion. (See #457)
[ ] Attach pre-build script to plugin_package installation process, that generates a text file (or other suitable file type) which includes the installation time of that certain package. If this installation time is newer than the modification time of registered_plugins.json of that package (if it even exists), then run / rerun create_plugin_registries to ensure GeoIPS is in a correct / valid state. (See #647)
All bullets in the list above that don't have a referenced issue are either described in their entirety above or will most likely not be addressed / relevant in the future. For example, the point talking about geoips test unit-test doesn't have an issue as that functionality will likely be removed.
Requested Update
Description
This issue was created due to lingering comments on the #444 PR. Since that PR is a dependency of other PRs like #465 and #478 , we are going to wait to make those updates and finish them up in a separate PR once those have been merged. Since none of the comments address actual bugs, this is largely just a refactoring change and is good to wait until we merge the aforementioned PRs.
Checklist for Completion
Add to the following list for all comments that still need addressing.
pkg
in CLI Unit tests.extend(list)
rather than+= list
. Personally I say we keep it as+=
all_possible_subcommand_combinations
for CLI Unit Tests (See #648)all_possible_subcommand_combinations
to something more accurate. Some include all cases; some are stochastic. (See #648)geoips unit / unit-test
command rather thangeoips test unit-test
(not important right now as this functionality has been completely commented out)GeoipsExecutableCommand:_get_registry_by_interface_and_package
toPluginRegistry
classtabulate
portions ofGeoipsList
to reduce some code duplicationgeoips list
prints usage that is poorly formatted.registered_plugins.json
of that package (if it even exists), then run / reruncreate_plugin_registries
to ensure GeoIPS is in a correct / valid state. (See #647)All bullets in the list above that don't have a referenced issue are either described in their entirety above or will most likely not be addressed / relevant in the future. For example, the point talking about
geoips test unit-test
doesn't have an issue as that functionality will likely be removed.