-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix resolution process #53
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's interesting how a small fix ripples through all the test code. I think maybe I need to do a better job focusing on the markdown or we need to start pulling in more of the existing markdown as our tests....
IN any case, looks good! Thanks for fixing this!
// attr and attr_parent are both references. | ||
AttributeSpec::Ref { | ||
r#ref: r#ref.clone(), | ||
brief: if brief.is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all these options, you can make a helper method to clean this up.
fn clone_first_some<T : Clone>(first: &Option<T>, second: &Option<T>): Option<T> {
if first.is_some() {
first.clone()
} else {
second.clone()
}
Then all this code becomes:
AttribtueSpec::Ref {
...
brief: clone_first_some(brief, parent_brief),
examples: clone_first_some(examples, parent_examples),
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created the following GH issue #54
Yes looks like using the existing markdown files is our best option at this point. |
This PR fixes an issue in the resolution process when there are more than 1 level of
extends
.Note 1: The lineage computation is still incomplete and will be fixed in the next PR (see #52). This is not blocking.
Note 2: The current resolution process is iterative and far from optimal but still pretty fast with the current semconv registry. If speed becomes an issue, a graph could be construct from the
extends
andref
clauses, and a DFS resolution could be used to optimize the entire resolution process.