Move MetaWarning classes into new module #142
Conversation
codebasin/cli.py
Outdated
There was a problem hiding this comment.
Not sure if it's just me, but the naming of this module doesn't seem to really fit its contents?
It's called cli.py but it contains logging stuff, and if we're moving things around, would it make sense to name it something that maps better? (log_utils.py?)
Otherwise, refactors and new tests look good to me
There was a problem hiding this comment.
Ah, I can see the confusion. I'll explain my logic, and you can tell me if it makes sense 😆.
The thinking behind this "cli" module was that it would contain things that were only useful for the user-facing command-line interface, but which might be shared across commands (e.g., the Formatter is currently used by both codebasin and cbicov).
I was basically looking for a way to separate things that we use to build the user-facing CLI from the modules and interfaces that we (eventually) want people to be able to use when importing the codebasin package into another script.
Maybe it would be better to have this as something like a cli package, containing a logging.py? Or an _internal package containing a logging.py? I'm definitely open to suggestions here.
There was a problem hiding this comment.
After offline discussion, we're considering using _detail to align with the C++ idea of "implementation details".
There was a problem hiding this comment.
I've renamed this module from "cli" to "_detail.logging" in 9108dce. Please let me know if it's not what you expected.
Pennycook
changed the title
Moving them out of __main__.py will make it easier to re-use them across different command-line interfaces, and may make them easier to test. While moving them, I also added the missing docstrings. Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
This renaming is effectively two changes, based on code review feedback: - The module is moved into a separate _detail package, to highlight that its contents are an implementation detail and not part of CBI's public interface. - The module is renamed from "cli" to "logging", since the three classes it defines (Formatter, MetaWarning, WarningAggregator) are all related to logging. Signed-off-by: John Pennycook <[email protected]>
Related issues
Part of #36, #96.
The CLI is the least well-tested part of Code Base Investigator, because its legacy implementation lived outside of the
codebasin/package and was therefore difficult to test. Refactoring common CLI functionality into a dedicated module, using functions with well-defined interfaces, will make this functionality more reliable.Proposed changes
Formatter,MetaWarningandWarningAggregatorinto a new module.Formatter,MetaWarningandWarningAggregator.