Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 7 commits
79be3fd
c794d37
735b3ff
436a139
a9a4538
f6eb330
5a26ba2
998705e
7d8fc67
9badcdd
7b64dd1
be8921e
e206a1e
19771ae
0543c42
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 745 in conda/common/io.py
Codecov / codecov/patch
conda/common/io.py#L745
Check warning on line 747 in conda/common/io.py
Codecov / codecov/patch
conda/common/io.py#L747
Check warning on line 750 in conda/common/io.py
Codecov / codecov/patch
conda/common/io.py#L750
Check warning on line 752 in conda/common/io.py
Codecov / codecov/patch
conda/common/io.py#L752
Check warning on line 797 in conda/common/io.py
Codecov / codecov/patch
conda/common/io.py#L796-L797
Check warning on line 801 in conda/common/io.py
Codecov / codecov/patch
conda/common/io.py#L801
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:
This is unwieldy, ideally the render function should be smart enough to figure it out for us (much like how
print
andrich
simply figure it out for us):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), whereasconda 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 printstr(value)
. Therefore there will need to be someif: 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 thecontext
, nor should it contain any logic to determining what is rendered.I expect
render
to know that astr
is a string and how to print it (usingstring_view
). It should also know that adict
is a mapping and how to print it (usingdetail_view
).I expect something like the following to happen:
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.
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.
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.
Check warning on line 808 in conda/common/io.py
Codecov / codecov/patch
conda/common/io.py#L807-L808
Check warning on line 811 in conda/common/io.py
Codecov / codecov/patch
conda/common/io.py#L811
Check warning on line 813 in conda/common/io.py
Codecov / codecov/patch
conda/common/io.py#L813
Check warning on line 817 in conda/common/io.py
Codecov / codecov/patch
conda/common/io.py#L816-L817