New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add reporter handler, output handler and reporter manager class structure to conda #13818
base: feature/reporters-backends
Are you sure you want to change the base?
Add reporter handler, output handler and reporter manager class structure to conda #13818
Conversation
CodSpeed Performance ReportMerging #13818 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I would just like to see some more comments in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add some more tests and get the code coverage for this new code closer to 100%.
"stdout": StdoutHandler(), | ||
} | ||
|
||
def render(self, name, data, **kwargs) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the usage here is intended to be:
manager = ReporterManager(...)
manager.render("string_view", "Hello world!")
manager.render("detail_view", {"foo": true})
This is unwieldy, ideally the render function should be smart enough to figure it out for us (much like how print
and rich
simply figure it out for us):
manager = ReporterManager(...)
manager.render("Hello world!")
manager.render({"foo": true})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, Ken!
The reason we decided to have two methods (one printing strings, one tabular data) in the first place is because a single conda command prints data in different forms depending on the flags used with it.
For example, conda info --base
simply prints a string (the root prefix), whereas conda info
prints tabular data (data printed in two columns; key, value pairs).
You are suggesting that reporter_manager.render(data)
should itself look at the context, flags passed etc, and print the data in the appropriate format.
For example the render method will find that --base
flag has been passed, so instead of printing "key: value", it will print str(value)
. Therefore there will need to be some if: else:
logic in the render method.
But the render method in the ReporterManager class is supposed to be a general class method that can be used by any conda command.
It is possible that some other conda command used with --base flag
DOES NOT print a string, but a key-value pair.
Therefore the render
method should not be responsible for everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I do not expect the render
method to know anything about the context
, nor should it contain any logic to determining what is rendered.
I expect render
to know that a str
is a string and how to print it (using string_view
). It should also know that a dict
is a mapping and how to print it (using detail_view
).
I expect something like the following to happen:
def conda_info(args):
info = {...}
if args.base:
# conda info --base
manager.render(info["base"])
elif args.envs:
# conda info --envs
manager.render(info["envs"])
elif ...:
...
else:
# conda info
manager.render(info)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, Ken!
I now understand your suggested changes better. Thank you.
I expect render to know that a str is a string and how to print it (using string_view). It should also know that a dict is a mapping and how to print it (using detail_view).
The render method is intended to point to a type of "template". The methods that we’ve defined on the ReporterHandler classes are these templates. For now, we only have "string_view" and "detail_view" which depend on the data type they receive.
But not all of these "templates"/"views" will necessarily depend on the data type passed in. Therefore we do not want to create any map between which data type was passed in and which "view" method gets called.
Does that make the reasoning behind this implementation clearer? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But not all of these "templates"/"views" will necessarily depend on the data type passed in.
If that's the case, a compelling example is necessary to sway my opinion. The implementation and the arguments for why we want it this way look and sound like we are over engineering. It's ok for us to over engineer but we need to have documented examples for why we are doing this.
In any case, I expect the vast majority of renders to be based on the data type. So the format/template name should at least be an optional key and when not provided it is determined from the data type.
Description
This PR adds the
ReporterHandler
andOutputHandler
abstract classes and theReporterManager
class to conda.This structure will be used to update the output of various conda commands to use the new reporter setting.
resolves #13814.
Checklist - did you ...
news
directory (using the template) for the next release's release notes?