Move rule characters into class attributes of TerminalReporter

[doerwalter]

Move characters used for drawing horizontal rules into class attributes of TerminalReporter.

This allows those characters to be replaced by plugins (i.e. with Unicode box drawing characters) and documents which characters are used.

[bluetech]

Thanks for splitting this! A couple of questions to start with:

1. How are plugins intended to replace the characters? Is it subclassing TerminalReporter, replacing it entirely, or mutating the class attributes, or something else?
2. You add both class attributes hrule_single etc. and methods write_hrule_single() etc. Which of these is the intended customization point?

[doerwalter]

Thanks for splitting this! A couple of questions to start with:

1. How are plugins intended to replace the characters? Is it subclassing TerminalReporter, replacing it entirely, or mutating the class attributes, or something else?

I can imagine all of those variants.

2. You add both class attributes hrule_single etc. and methods write_hrule_single() etc. Which of these is the intended customization point?

I think for simple customizations replacing the characters (either directly or by overrwriting the attributes in a subclasses) is the way to go. But I can imagine bars that are done by using a different background color or by drawing a frame around the title. For this overwriting the method would be necessary.

But then the question becomes whether write_hrule_double() etc. are the best names for the methods. What if someone wants to have different rules for different situations? Maybe the names should describe some kind of hierarchy like write_title_level1(), write_title_level2()? Or maybe each distinct call needs it's own write_hrule_...() implemenation?

[nicoddemus]

I think for simple customizations replacing the characters (either directly or by overrwriting the attributes in a subclasses) is the way to go. But I can imagine bars that are done by using a different background color or by drawing a frame around the title. For this overwriting the method would be necessary.

Indeed, that was my understanding as well.

But then the question becomes whether write_hrule_double() etc. are the best names for the methods. What if someone wants to have different rules for different situations? Maybe the names should describe some kind of hierarchy like write_title_level1(), write_title_level2()? Or maybe each distinct call needs it's own write_hrule_...() implemenation?

Those are good points. To me the level_1, level_2 makes sense, seems similar to different "headings" in markups.

[doerwalter]

[...]

But then the question becomes whether write_hrule_double() etc. are the best names for the methods. What if someone wants to have different rules for different situations? Maybe the names should describe some kind of hierarchy like write_title_level1(), write_title_level2()? Or maybe each distinct call needs it's own write_hrule_...() implemenation?

Those are good points. To me the level_1, level_2 makes sense, seems similar to different "headings" in markups.

OK, then write_hrule_double() would probably be write_hrule_level1(). But I don't know whether write_hrule_single() or write_hrule_lower() would be level 2. And write_hrule_error() doesn't fit into this hierarchy anyway.

[graingert]

write_hrule(level: LevelEnum) -> int:?

[doerwalter]

write_hrule(level: LevelEnum) -> int:?

Nice idea. But I'm not sure what the return value of type int is supposed to represent.

And if pytest decides to add new LevelEnum values, plugins might implement a fallback for values they don't know, which makes the whole thing somewhat backwards compatible.

I'm no longer sure if write_hrule... is the correct name for this. Maybe something like write_header()?

[graingert]

To return the int from https://docs.python.org/3/library/io.html#io.TextIOBase.write

[doerwalter]

Ah, OK. Currently none of the write_...() methods return this value. I don't know if there would be any use for it.

[nicoddemus]

I like def def write_header(level: int = 0) -> None:.

@graingert do you see a use case for returning an int from that method?

[graingert]

Ok -> None: is good too

[doerwalter]

Now the question remains whether level really is an int or an enum with distinct but unrelated values.

[nicoddemus]

Now the question remains whether level really is an int or an enum with distinct but unrelated values.

Not sure either way.

Would like to hear @bluetech's opinion before moving further into the discussion however.

[bluetech]

My concern is that this adds a way to customize TerminalRepoter, but TerminalReporter is not really designed for customization.

For a library like pytest, we need to be careful about public interfaces and the stability guarantees we make for them. For a class like like TerminalReporter, I can think of 3 ways we can approach customization. We need to decide which approach we want, and afterwards to make sure we have the right design for it.

Subclassing

In practice it means writing a class which inherits from TerminalReporter, overriding various methods and attributes, and replacing the terminalreporter plugin with an instance of that subclass. This is what pytest-sugar does.

Currently, TerminalReporter is not designed for this. A class designed for subclassing needs careful work, first to distinguish "private", "protected" and "public" visibility (to use the Java terms; Python provides the @final decorator for this), and for the protected methods, make them documented, convenient and stable. TerminalReporter doesn't

Also, TerminalReporter is not even exported in the pytest namespace, so just naming it requires accessing the _pytest namespace.

Monkeypatching

In practice it means obtaining the terminalreporter plugin, and assigning various attributes on it.

Similarly to subclassing, if we want to sanction this method, we need to clearly document which attributes are monkeypatchable, and make them convenient and stable.

To me this seems not very elegant, I think plugin authors would prefer other methods, and regular users are very unlikely to do it on their own.

Also, it is mostly limited to overriding simple attributes, suggesting to monkeypatching methods I think is too much. So this approach is less flexible.

Hooks

This is the official sanctioned method that pytest offers for customizing things. It is stable and well-defined.

Of course, TerminalReporter itself is just a plugin, and does everything with hooks, so one can potentially replace the entire thing, but that is not practical.

So the more practical thing is to have TerminalReporter dispatch its own hooks that plugins can use. It already delegates a few tasks to hooks it dispatches, with default implementations, but not extensively: pytest_terminal_summary, pytest_report_header.

[bluetech]

Regarding the more specific discussion:

I think for simple customizations replacing the characters (either directly or by overrwriting the attributes in a subclasses) is the way to go. But I can imagine bars that are done by using a different background color or by drawing a frame around the title. For this overwriting the method would be necessary.

Generally more customization points mean more mental and maintenance overhead, so my preference would be to stick to one level, unless they are aimed at different audiences (e.g. plugins and users). If the method is more general and just as viable, I would provide only that.

But then the question becomes whether write_hrule_double() etc. are the best names for the methods. What if someone wants to have different rules for different situations? Maybe the names should describe some kind of hierarchy like write_title_level1(), write_title_level2()? Or maybe each distinct call needs it's own write_hrule_...() implemenation?

I don't have a detailed opinion but I do prefer the the names to be as semantic as possible instead of hrule_lower etc.

[nicoddemus]

My concern is that this adds a way to customize TerminalRepoter, but TerminalReporter is not really designed for customization.

@bluetech you are definitely right in your points here.

The current situation is that pytest output is not really customizable, so plugins that try to customize it usually fallback to subclassing TerminalReporter. This is not officially sanctioned by pytest, although we do try to avoid breaking known plugins like pytest-sugar, so adding a few attributes there is not really making matters worse, is just doing more of the same, but I agree the "more of the same" is not good/far from ideal.

As you say, hooks is the way to go method for customization in pytest and there are already hooks dedicated to customize terminal output, so we might consider that approach indeed.

If we go with hooks, I don't see a clear way to customize the actual writing of the separators (as one would with subclassing and writing their own routine), only the characters used to draw the separators, similar to what we have with pytest_report_teststatus. I think that is fine though. We could then have:

def pytest_terminal_separators(config: Config) -> Dict[str, str]:
    ...

Which would return the separators used to draw each "level", for example the default would be:

{
    "heading_1": "=",
    "heading_2": "-",
    "heading_3": "_",
    "error": "!",
}

[doerwalter]

But this version wouldn't allow doing something fancier. Why can't the hook do it's own drawing? Because this would have to callback into some basic drawing functionality?

[nicoddemus]

Why can't the hook do it's own drawing? Because this would have to callback into some basic drawing functionality?

That's a possibility indeed; in fact we already have a hook that does provide the terminalreporter to hook implementations to do as they please:

def pytest_terminal_summary(
    terminalreporter: "TerminalReporter", exitstatus: "ExitCode", config: "Config",
) -> None:

[bluetech]

As per the discussion above, this feature would need some more design work, so I'll close this PR. Thanks @doerwalter.