Conversation
casey
left a comment
casey
left a comment
There was a problem hiding this comment.
Nice, this is a definite improvement. See comments!
| let mut doc_attr: Option<&str> = None; | ||
| let mut groups = Vec::new(); | ||
| for attribute in attributes { | ||
| attributes.ensure_valid_attributes( |
There was a problem hiding this comment.
I think we should keep the error checking in the else below, otherwise the set of allowed attributes is in two places, here, where we call ensure_valid_attributes, and below, in the if / else statement.
There was a problem hiding this comment.
If we keep the error checking here, then we would have pretty similar error-generation code here and in the ensure_valid_attributes method. I added ensure_valid_attributes as a way of de-duplicating attribute error generation code. I modified the code below to add an unreachable and hopefully make it clearer what's going on.
| inner: BTreeSet<Attribute<'src>>, | ||
| } | ||
|
|
||
| impl<'src> Serialize for AttributeSet<'src> { |
There was a problem hiding this comment.
Can we avoid needing to write a custom a serialize implementation if we make the struct:
pub(crate) struct AttributeSet<'src>(BTreeSet<Attribute<'src>>);
I think in that case serde will serialize the struct as just the inner contents.
| } | ||
| } | ||
|
|
||
| pub(crate) fn count(&self) -> usize { |
There was a problem hiding this comment.
I think we should just call this len, since that's what native collections use.
|
|
||
| pub(crate) fn contains(&self, target: AttributeDiscriminant) -> bool { | ||
| self.inner.iter().any(|attr| { | ||
| let discriminant: AttributeDiscriminant = attr.into(); |
There was a problem hiding this comment.
Let's add a fn discriminant(&self) -> AttributeDiscriminant { self.into() } function to Attribute, so we can avoid needing to use type inference to do the conversion.
| self.inner.iter() | ||
| } | ||
|
|
||
| pub(crate) fn private(&self) -> bool { |
There was a problem hiding this comment.
I think we should avoid one-off helpers like this, unless they do something more complex, since doing attributes.contains(AttributeDiscriminant::Private) isn't so bad.
| Ok(None) | ||
| } else { | ||
| Ok(Some((token.unwrap(), attributes.into_keys().collect()))) | ||
| let attribute_set = AttributeSet::from_iter(attributes.into_keys()); |
There was a problem hiding this comment.
Can you do collect here instead of from_iter?
There was a problem hiding this comment.
Not quite sure what code change you want to see here.
| None | ||
| } | ||
| }); | ||
| let extension = self |
There was a problem hiding this comment.
I think the original version of this is actually better, since we don't duplicate which attribute we're looking for.
| if let Attribute::Doc(doc) = attribute { | ||
| return doc.as_ref().map(|s| s.cooked.as_ref()); | ||
| } | ||
| if let Some(Attribute::Doc(doc)) = self.attributes.get(AttributeDiscriminant::Doc) { |
There was a problem hiding this comment.
This is kind of a weird one. This if let will only match if both self.attributes.get returns Some, and the returned attribute is an Attribute::Doc, if the code is correct, of course, both or neither will be the case.
I think a more scrupulous correct version would be:
if let Some(attribute) = self.attributes.get(AttributeDiscriminant::Doc) {
if let Some(Attribute::Doc(doc)) else {
unreachable!();
}
}
Although that's a little crazy, so I think maybe the original on the left is better.
| } | ||
|
|
||
| for attribute in &self.attributes { | ||
| for attribute in self.attributes.iter() { |
There was a problem hiding this comment.
We should implement IntoIterator for &AttributeSet so we can avoid needing to call .iter().
neunenak
force-pushed
the
attributeset
branch
from
2434438 to
73a6f51
Compare
neunenak
force-pushed
the
attributeset
branch
from
fe755a7 to
b647045
Compare
|
@casey were there any more tweaks you wanted to make to this PR? It looks like some subsequent code merges have caused conflict with this PR, I'll try to fix them unless you had something specific you were trying to do with your commits. |
casey
left a comment
casey
left a comment
There was a problem hiding this comment.
Fixed the merge conflicts. I think this is fine. I have slight misgivings about needing to keep the sets of valid attributes in the ensure_valid_attributes call and match statement in sync, but I can't actually think of a way to improve it.
2 participants
Replace the several places that directly use a
BTreeSet<Attribute>with a wrapper data structureAttributeSet, that provides some convenience methods for working with attributes.