Repository navigation
make _DomainsAction an external attribute of _internal.cli.cli_utils - #10514
Conversation
| from certbot.compat import os | ||
| from certbot.display import util as display_util | ||
| from certbot._internal import cli | ||
| from certbot._internal.cli import cli_utils |
There was a problem hiding this comment.
if I understand correctly, the reason to stick the from certbot._internal.cli.cli_utils import _DomainsAction in cli/__init__.py is so we could keep doing imports here as cli._DomainsAction. If we're going ahead and switching this here to import cli_utils, is there a reason to keep the import line in cli/__init__.py?
There was a problem hiding this comment.
i think the reason for the from certbot._internal.cli.cli_utils import _DomainsAction in cli/__init__.py is because _DomainsAction is used in cli/__init__.py. i think throughout the certbot codebase (and maybe in the python ecosystem more generally?) imports like from certbot._internal.cli import cli_utils and uses like clI_utils._DomainsAction is more common, but that's not what cli/__init__.py does for whatever reason
i also searched for other places we're using "private" functionality imported in cli/__init__.py like this and didn't find any other than the import i'm changing here
$ git --no-pager grep -e 'cli import _' -e 'cli._'
certbot/src/certbot/_internal/tests/plugins/webroot_test.py: action=cli._DomainsAction, default=[])
There was a problem hiding this comment.
Ah, I did forget about the call in __init__.py when I wrote this comment. 🤦
Yeah, agree it's more common elsewhere, I assumed it was a leftover from when we refactored cli.py into having multiple sub-files. I assumed that was the only reason to import it at all in __init__py, which isn't the case, and I don't actually mind that style of import there, I was just worried (incorrectly) that we were importing it for no reason now that this style is changed.
as i mentioned at #10509 (comment), i didn't love how the tests were using
_DomainsActionwhen i think the leading underscore suggests the class is internal to the cli/cl_utils modulethis PR fixes that
also, i don't think this PR requires two reviews