Add a summary() function for configuration summarization #4649
Conversation
dcbaker
changed the title
|
I'm considering this RFC at the moment because I'd like some feedback on the API portion, namely, does it make more sense to use the special case dict structure, or to allow |
dcbaker
force-pushed
the
summary-function
branch
from
34891d6
to
7b69adb
Compare
|
I switched to the keyword argument variant, which always treats dicts as dicts with no special meaning based on their level. I think I like this better as it's more consistent. |
|
I think from an API point of view I prefer the "take arbitrary kwargs for sections and always print dictionaries" variant to the dict-inside-dicts variant. Just looks nicer and cleaner and also discourages nesting of sections. |
|
We're in agreement then, and should ask @jpakkane how he feels about the api? |
|
Interestingly I just manage to beat you by one day: #4643 It's a lot simpler, though... |
|
Which one is simpler? |
|
The one thing the other MR does that this one does not is to store the summary per subproject and print all of them in one go at the very end (with the master project last and subprojects in alphabetical order). |
|
That's how this one is supposed to work (minus the alphabetical order), but I change that. |
|
Apparently there are bugs :) |
dcbaker
force-pushed
the
summary-function
branch
2 times, most recently
from
061a997
to
f9b3cd6
Compare
|
Updated to sort subprojects alphabetically, print the main project last, and not print subproject summaries during subproject initialization. |
|
The thing I don't like in this PR is the free-form keyword arguments. That means we cannot extend this function with other options. For example we'll never be able to add summary(..., colored : true) because that would print "colored=true" instead of being a setting of the summary function. I would rather have nested dict (with max depth to 2) to be future-proof. From a readability POV, it makes no difference, it looks even more consistent to me. Allow multiple positional arguments that must be all dictionaries: summary(
{'Backend' : 'OpenGL'},
{'Server' : sec1},
{'Client' : sec2},
{'Misc' : sec3},
)Also that allows the incrementally construct the summary: summary = [{'Backend' : 'OpenGL'}]
if something
summary += {'foo' : 'bar'}
endif
summary(summary)So it would be the same to have 1 list positional argument, or N dict positional arguments. Many functions works that way already. |
|
I tried that originally, and I don't like it because it makes dicts mean 2 things, for two levels of nesting they mean "indent this" but after that they don't: foo = {'one' : {'two' : {'three' : {'four': 'foo'}}}}
summary(foo)would print as: but foo = {'one' : {'two' : {'three' : 'foo'}}}prints as: Which I feel violates least-surprise. I agree that maintaining keywords for API use would be better, so how about: summary({'Backend' : 'OpenGL'}, sections : {'Server' : sec1, 'Client' : sec2, 'Misc': sec3})Which remove the free-form keywords, but keeps the behavior of nested dictionaries consistent. |
That's nested dictionaries too, I think the advantage of my proposal is that sections are in a list, so user controls the ordering.
Of course put like that it's confusing, but with intermediary variables and a bit of indentation, it's not that bad. Taking your initial example: |
|
One thing I would like to do with keyword arguments, is enabling some automatic sections (doesn't need to be done in initial PR). One example that comes to mind is listing all subprojects with a YES/NO value to tell if they worked. summary(subprojects: true)Would print: Another extention can that be done later, is passing dependency objects libfoo = dependency('foo', required : get_option('foo_feature'))
summary({'Dependencies' : [libfoo, libbar]})Would print: All of this doesn't need to be implemented immediately, but I would like to make sure we keep in mind that this will likely be extended in the future, so we have to design it flexible. |
|
Do meson's dicts have an ordering guarantee? Is that behavior just leaking from python 3.7, or do we mean to have it? |
|
There is no ordering guarantee. |
|
I guess that leaves us with either @xclaesse's list of lists (which I'm not a huge fan of, but I could live with I think), making dictionaries ordered (Because python guarantees it now, people will start relying on it so it might be worth us doing), or going to a more complicated object, maybe a module? Does anyone have preferences? |
|
(Just thinking out loud here...) Maybe we should take a step back and revisit some of the original ideas floating about in e.g. #757. I think we're perhaps trying to do too much at once here, trying to squeeze everything into a single How about something where a single |
|
Or even simpler: let there be only a single |
We already have to support multiple I personally still prefer my proposal from #4649 (comment), but if people prefer @tp-m's proposal, I've got no strong opinion against it. |
|
@tp-m, could you give me a psudeo-code example of what you're thinking? |
I guess something like that: sec1 = {'driver' : 'foobar', 'OS' : 'Linux', 'API' : 1.7}
sec2 = {'driver' : 'dive comp', 'OS' : 'Minix', 'API' : 1.1.2}
sec3 = {'with' : {'mesa' : true, 'gbm' : false}}
summary({'Backend' : 'OpenGL'})
summary(sec1, title : 'Server')
summary(sec2, title : 'Client')
summary(sec3, title : 'Misc') |
Something like this I think: Single section: Multiple sections: (not sure what the behaviour of the nesting should be here in the Misc section, but that's details ;)) |
|
Okay, I'll update this today and see how the multiple calls to summary works out. |
|
Since this is a new function, please also add this 'summary': self.func_do_nothing,to the |
I think it'd be more user-friendly to merge instead, if the same section name is specified twice, and only warn or error out if the same dict key appears twice in the same section. For example when subdirectories are used, it is much more convenient to add items to the status in a few different places then to manually collect everything in a dictionary and only call summary() at the end. And the same goes for conditional output, e.g. architecture dependent. I tried using this with systemd, and the output looks like this: It is not bad. It is definitely much less work than the current hand-crafted summary we have. Nevertheless, it is not as readable as the hand-crafted yet, and I wouldn't want to switch from the handcrafted status to this until some things are improved.
I'd prefer this to be something like "<project-name> <project-version> summary" instead. For projects that don't use subprojects this line is noise. Or maybe make this configurable though a keyword argument. For the dictionary, I think just using the default dict formatter is not going to be good enough.
It would be better to print the strings without quotes. Many things are internally strings but are never displayed quoted, like version numbers. Having the quotes on some settings or not others is visually displeasing. The same goes for stuff like "(not set)" or similar — they must be displayed without quotes.
For strings with quotes, it would be great if they could be split at newlines and aligned to the "=" sign. I.e display the first version as: I also tried a version with a list:
This looks better out of the box, but is not very readable. Breaking the list up and aligning to "=" would be great. Maybe use color for the section headers? |
|
I pushed my test as https://github.com/keszybz/systemd/pull/new/meson-summary in case anyone wants to play with it. |
Agreed, I'll change that.
Agreed. If we go with
Agreed. When the value is a simple string the quoting is just noise. But when the value is array/dict then it could be needed maybe.
I don't think we should try to parse strings, the point here is project would have to use an array of string instead, as you did below.
Agreed, I like how systemd breaks that list into lines, that could be the default behaviour when value is a list.
Agreed, but I'm not sure which color :) Speaking about colors, I also wanted a mode where booleans would turn into YES/NO in green/red, but that can be future improvement. |
xclaesse
force-pushed
the
summary-function
branch
2 times, most recently
from
b86c1d2
to
51dcea2
Compare
|
@keszybz I updated the PR with those suggestions. I dropped 2 use-cases to simplify the code:
I'm considering adding |
xclaesse
force-pushed
the
summary-function
branch
from
51dcea2
to
8252dcc
Compare
xclaesse
force-pushed
the
summary-function
branch
3 times, most recently
from
7165326
to
4529d8b
Compare
Added all those variants now, in a separate commit in case we prefer merge the initial commit first. |
Done, with |
|
Very pretty ;) |
xclaesse
force-pushed
the
summary-function
branch
2 times, most recently
from
ada826b
to
b265ddb
Compare
Based on patch from Dylan Baker. Fixes mesonbuild#757
xclaesse
force-pushed
the
summary-function
branch
from
b265ddb
to
a4bb092
Compare
xclaesse
changed the title
|
@jpakkane, this looks good to me. Since it has frontend language changes I figured you'd like tot ake a peak before this lands. |
| mlog.log(' ', mlog.bold(section)) | ||
| for k, v in values.items(): | ||
| indent = self.max_key_len - len(k) + 3 | ||
| mlog.log(' ' * indent, k + ':', v[0]) |
This has been requested multiple times in the mesa/xorg community, and
apparently in the GStreamer community as well.
This allows a configuration summary to be printed at the end of the configuring phase.
For example:
Which would print something like:
Configuration Summary: Backend = OpenGL Server: driver = foobar OS = Linux API = 1.7 Client: driver = dive comp OS = Minix API = 1.1.2 Misc: with = {'mesa : true, 'gbm' : false}Fixes #757