feat: nicer skip context for macro/attribute #5459
Conversation
src/skip.rs
Outdated
|
|
||
| impl SkipNameContext { | ||
| pub(crate) fn append(&mut self, values: Vec<String>) { | ||
| self.values.extend(values); |
There was a problem hiding this comment.
once self.all=true does it make sense to extend? Could this be:
| self.values.extend(values); | |
| if !self.all { | |
| self.values.extend(values); | |
| } |
There was a problem hiding this comment.
Agreed - I refactored the inner types, but made sure the same logic applies.
src/visitor.rs
Outdated
| for macro_selector in config.skip_macro_invocations().0 { | ||
| match macro_selector { | ||
| MacroSelector::Name(name) => macro_names.push(name.to_string()), | ||
| MacroSelector::All => skip_context.all_macros = true, | ||
| MacroSelector::All => skip_context.macros.set_all(true), | ||
| } | ||
| } | ||
| skip_context.update_macros(macro_names); | ||
| skip_context.macros.append(macro_names); |
There was a problem hiding this comment.
Along with the comments above, I wonder if it makes sense to avoid calling to_string when "*" is in the skip_macro_invocations list by making the following update:
let contains_all = config.skip_macro_invocations().0.any(MacroSelector::is_all);
if contains_all {
skip_context.macros.set_all(true);
} else {
skip_context.macros.append(config.skip_macro_invocations().0.collect());
}The code might need to be written a little differently, but I hope the general idea is understood.
There was a problem hiding this comment.
I think realistically it's unlikely we see a case that makes it worth this optimization, at the cost of reading the vec twice. My expectation is that "*", if given, will be the only string in the vec, as it makes all other names redundant. The unit tests that check combinations of the two are just for completeness.
|
Updated to use the Refactored the inner type to an enum, since it matches the behaviour nicely. |
|
@tommilligan did you push the changes? I don't think I"m seeing anything different than last time. I'm also not seeing any impls for |
Sorry - now pushed. Was working on the train, looks like the connection failed and I didn't notice, sorry. |
There was a problem hiding this comment.
This lgtm but will hold off on merging in case you want to take a final pass @ytmimi
| @@ -775,10 +775,10 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { | |||
| for macro_selector in config.skip_macro_invocations().0 { | |||
| match macro_selector { | |||
| MacroSelector::Name(name) => macro_names.push(name.to_string()), | |||
| MacroSelector::All => skip_context.all_macros = true, | |||
| MacroSelector::All => skip_context.macros.skip_all(), | |||
There was a problem hiding this comment.
I'd think we could technically break after this arm because if we reach it then it's a bit moot whether there's additional entries. However, it shouldn't really matter. I don't even a super villain could be pathological enough to make it matter
| MacroSelector::All => skip_context.macros.skip_all(), | |
| MacroSelector::All => { | |
| skip_context.macros.skip_all(); | |
| break; | |
| } |
There was a problem hiding this comment.
@tommilligan thanks for your work on this! I like the update and I'm ready to get this pulled in!
If you can rebase this on master then I can merge, otherwise @calebcartwright can merge it.
Follow up from #5347 (comment) in discussion with @ytmimi
Refactors the way named items are skipped such that:
This should result in no change in behaviour.