Skip to content
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

Open
wants to merge 15 commits into
base: feature/reporters-backends
Choose a base branch
from

Conversation

ForgottenProgramme
Copy link
Contributor

@ForgottenProgramme ForgottenProgramme commented Apr 19, 2024

Description

This PR adds the ReporterHandler and OutputHandler abstract classes and the ReporterManager 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 ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@ForgottenProgramme ForgottenProgramme requested a review from a team as a code owner April 19, 2024 08:10
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Apr 19, 2024
Copy link

codspeed-hq bot commented Apr 19, 2024

CodSpeed Performance Report

Merging #13818 will not alter performance

Comparing ForgottenProgramme:reporter-handler (0543c42) with main (5b11a8c)

Summary

✅ 21 untouched benchmarks

Copy link
Contributor

@travishathaway travishathaway left a 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.

conda/common/io.py Outdated Show resolved Hide resolved
conda/common/io.py Show resolved Hide resolved
conda/common/io.py Show resolved Hide resolved
tests/common/test_io.py Show resolved Hide resolved
Copy link
Contributor

@kenodegard kenodegard left a 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%.

conda/common/io.py Outdated Show resolved Hide resolved
conda/common/io.py Outdated Show resolved Hide resolved
conda/common/io.py Outdated Show resolved Hide resolved
tests/common/test_io.py Outdated Show resolved Hide resolved
conda/common/io.py Outdated Show resolved Hide resolved
conda/common/io.py Outdated Show resolved Hide resolved
@travishathaway travishathaway self-requested a review April 23, 2024 04:54
@ForgottenProgramme ForgottenProgramme changed the base branch from main to reporters-backends April 25, 2024 11:32
conda/common/io.py Outdated Show resolved Hide resolved
"stdout": StdoutHandler(),
}

def render(self, name, data, **kwargs) -> None:
Copy link
Contributor

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})

Copy link
Contributor Author

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.

Copy link
Contributor

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)

Copy link
Contributor Author

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? :)

Copy link
Contributor

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.

tests/common/test_io.py Outdated Show resolved Hide resolved
tests/common/test_io.py Outdated Show resolved Hide resolved
tests/common/test_io.py Outdated Show resolved Hide resolved
tests/common/test_io.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Status: 🏗️ In Progress
Development

Successfully merging this pull request may close these issues.

Create the class structure used for reporter handlers and output handlers
4 participants