Skip to content

Rename csv_utils to _csv_utils and fix imports#3738

Open
Ulincsys wants to merge 1 commit intomainfrom
fix-multicommand-csv-tasks
Open

Rename csv_utils to _csv_utils and fix imports#3738
Ulincsys wants to merge 1 commit intomainfrom
fix-multicommand-csv-tasks

Conversation

@Ulincsys
Copy link
Contributor

Description

  • Rename csv_utils.py to _csv_utils.py
    • Any PY file in the augur/application/cli directory is assumed by Click to be a CLI multicommand submodule, unless that file's name begins with an underscore _. The csv_utils.py file is not a multicommand submodule, and so must be prefixed with an underscore to exclude it from dynamic loading during multicommand processing.
  • Fix util imports
    • The augur.application.cli.tasks multicommand submodule previously imported clear_rabbitmq_messages and raise_open_file_limit from the base backend multicommand submodule, but they have since moved to augur.application.cli._cli_util

With this patch, running augur --help successfully loads all multicommand submodules and displays the global help message.

This PR fixes #3627

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Ulincsys <ulincsys@gmail.com>
Copy link
Collaborator

@shlokgilda shlokgilda left a comment

Choose a reason for hiding this comment

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

LGTM

@MoralCode
Copy link
Contributor

I dont like the idea of renaming a shared utility like this because of a unique issue with click. I didnt see another way until i read:

but they have since moved to augur.application.cli._cli_util

This makes me think: could we import the needed csv utils in this already-prefixed CLI util file instead and then wherever its needed in the CLI can import it from that already-prefixed file?

@Ulincsys
Copy link
Contributor Author

I dont like the idea of renaming a shared utility like this because of a unique issue with click.

Is there a specific objection you have as to why renaming the file is improper?

It doesn't really matter to me how this issue gets resolved, I just took the approach that required the least changes to achieve the desired effect.

@MoralCode
Copy link
Contributor

Is there a specific objection you have as to why renaming the file is improper?

A bad assumption that this utils file existed somewhere else in the code and that we were having the CSV module dictate filenames outside of its module.

I think this is fine for now. Long term I think id like to find a better way to discover click modules (maybe an AST parsing based system to lightly parse each file?), but this is fine for now

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.

Bug: augur --help crashes with AttributeError: module 'csv_utils' has no attribute 'cli'

3 participants