-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/uppsf 6584 extend the to string transformer #125
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
base: main
Are you sure you want to change the base?
Feature/uppsf 6584 extend the to string transformer #125
Conversation
vlasakievft
left a comment
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.
suggestion: Maybe would be a good idea to add tests for:
InNumbers
Recommended
RecommendedList
|
praise: The commits are organized in a very good way! |
libraries/to-string/go/transform.go
Outdated
| ) | ||
| var ErrUnknownKind = errors.New("unknown tree kind") | ||
|
|
||
| var toSeparate = []string{contenttree.HeadingType, contenttree.ParagraphType} |
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.
question: Is it logical to add contenttree.PullquoteType to toSeparate?
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.
Good catch. This wouldn't have solved the problem, but I revamped the spacing logic and should be ok now.
|
|
||
| return pq.Text, nil | ||
| case contenttree.InNumbersType: | ||
| def, ok := n.(*contenttree.Definition) |
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.
Issue: Here the node should be typecast to contenttree.InNumber and the title field on it should be used.
in, ok := n.(*contenttree.InNumbers)
if !ok {
return "", errors.New("failed to parse node to InNumbers")
}
resultChildred, err := transformChildren(in.GetChildren())
if err != nil {
return "", err
}
return fmt.Sprintf("%s %s", in.Title, resultChildred), nil
Then, we will need an additional case for Definition type.
| return transformNode(root.Body) | ||
| } | ||
|
|
||
| switch n.GetType() { |
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.
suggestion: we should add case: contenttree.TimelineEventChildType node for completeness.
The code works without it because in the default case we are calling n.GetChildren() which internally calls the GetChildren on the embedded node. For example,
func (n *TimelineEventChild) GetChildren() []Node {
if n.Paragraph != nil {
return n.Paragraph.GetChildren()
}
if n.ImageSet != nil {
return n.ImageSet.GetChildren()
}
return nil
}
So theoretically, we could get rid of all the checks where we return transformNode(n.GetEmbedded()).
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.
I think removing the transformNode(n.GetEmbedded()) would be the better approach here.
No need to burden the switch with more cases than needed. What do you think @todor-videv1?
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.
Two options here... if we are sure we can safely remove the n.GetEmbedded()... less is more, I'd advse to leave a comment in the code how the default case is dealing with these embeds though.
If we have even an ounce of doubt, add the additional case like Lokender suggests, and then when we fix the tests, we can do this enhancement.
6e308e3 to
cef1a0b
Compare
JIRA ticket