Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
CodSpeed Performance ReportMerging #7591 will not alter performanceComparing Summary
|
| fields.sort_unstable_by(|(name, _), (name2, _)| name.cmp(name2)); | ||
| sets.sort_unstable_by(|(name, _), (name2, _)| name.cmp(name2)); |
There was a problem hiding this comment.
I don't know how I feel about sorting options. It disallows grouping options that are semantically related (all resolver settings)
| impl OptionsMetadata for FormatOrOutputFormat { | ||
| fn record(visit: &mut dyn Visit) { | ||
| FormatOptions::record(visit) | ||
| } | ||
| } |
There was a problem hiding this comment.
This also feels less hacky now since we have a trait.
960622f to
5dc5178
Compare
| impl Visit for CollectOptionsVisitor { | ||
| fn record_set(&mut self, name: &str, group: OptionSet) { | ||
| self.groups.push((name.to_owned(), group)); | ||
| } |
There was a problem hiding this comment.
The old implementation only supports one level of nesting. This would have become a problem with lint.pydocstyle.option where we have two level nesting.
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
bc3127d to
effaf85
Compare
5dc5178 to
86a61b3
Compare
| } | ||
|
|
||
| fn generate_set(output: &mut String, set: &Set) { | ||
| writeln!(output, "### {title}\n", title = set.title()).unwrap(); |
There was a problem hiding this comment.
writeln! with trailing \n looks strange
There was a problem hiding this comment.
Agree but everything else feels verbose just to get two newlines
There was a problem hiding this comment.
(I typically do two writeln for this but don't feel strongly.)
effaf85 to
a5e2a14
Compare
86a61b3 to
2ee6747
Compare
a5e2a14 to
7a13d26
Compare
2ee6747 to
0fe9465
Compare
0fe9465 to
f63df7d
Compare

Summary
This PR refactors the
Optionsrepresentation.Current Reprsentation
Our
ConfigureOptionsmacro currently generates the following code (truncated):You can see how it generates a static array and
metadatareturns anOptionGroup(&slice)referring to the slice of the static array.Options can be nested (option-groups). The macro calls into the nested type's
metadatafunction to get theOptionGroupand adds aOptionEntry::Group()to its static arrayThis works well enough and the API is simple. However, it requires that the macro statically determines all options to be able to set the length of the array.
New Requirements
I'm about to introduce a new
lintsection to the configuration without removing the top-level options until the new configuration is stabilized. The simplest but very verbose approach is duplicating thelintoptions: Once at the top level and once under thelintsection. But that's rather verbose. Ideally, I want to use serde'sflattenfeature and write:While serde and schemars support this nicely, our options macro does not, and adding it is impossible with the current representation. The problem is that macros operate on a tokens stream only. They can't resolve types (at least to my knowledge). But that's what's necessary for
Optionsto statically determine the length of itsOPTIONSarray: It needs to expand theLintOptionsoptions intoOptions.New Representation
The new representation is inspired by serde and
tracing_subscribs'sVisittrait.The basic idea is to implement the metadata abstraction as a visitor pass (to avoid allocations).
OptionsMetadataOptionsMetadata::recordcallsrecord_fieldorrecord_setfor each optionwhere
Visitis defined asThis new representation has a few advantages:
OptionsMetadatatrait can be implemented manually for cases where the derive macro isn't flexible enoughOptionsMetadataprovides all relevant abstractions so that the macro is optional. It means you can now understand the system without understanding the macro.flattenbecomes a simpleNested::record(visit)call (instead of callingvisit.record_set(name, Nested::metadata()). Meaning we traverse into the sub-set without telling the visitor that it now enters a new set.The main downside of the new API is that writing visitors is awkward. This is evident by the longer implementations for
has,get, andDisplay. I think that's a fair tradeoff, considering that these methods are implemented once and that a similar API is provided to consumers.Test Plan
cargo dev generate-optionsproduces the exact same outputconfigcommand continues working