V1.6

[hasty]

Hefty reworking of spec parsing to be completely non-destructive

[hasty]

/gemini review

[gemini-code-assist]

Warning

Gemini encountered an error creating the summary. You can try again by commenting /gemini summary.

[gemini-code-assist]

Code Review

This pull request introduces a significant and commendable refactoring to make spec parsing completely non-destructive. The core of this change is the new overlay package and the addition of Clone() methods across the data model, which is a solid approach for this kind of task. The changes are extensive and generally well-executed. I've identified a few issues, primarily related to incorrect or incomplete Clone() implementations and some inconsistencies in the new traversal logic. Addressing these will help ensure the robustness and correctness of the new parsing mechanism.

[gemini-code-assist]

The Clone() method for DoubleBold incorrectly returns a *Bold instance instead of a *DoubleBold instance. This will lead to type errors and unexpected behavior when cloning DoubleBold elements.

Suggested change

	func (db *DoubleBold) Clone() Element {
	return &Bold{position: db.position, raw: db.raw, AttributeList: db.AttributeList.Clone(), Elements: db.Elements.Clone()}
	}
	func (db *DoubleBold) Clone() Element {
	return &DoubleBold{position: db.position, raw: db.raw, AttributeList: db.AttributeList.Clone(), Elements: db.Elements.Clone()}
	}

[gemini-code-assist]

The Clone() method for ShorthandAttribute initializes new slices for Roles and Options but fails to copy and clone the elements from the original slices. This results in slices of nil pointers, leading to data loss and potential panics. You should iterate over the original slices and clone each element individually.

Suggested change

	func (sa *ShorthandAttribute) Clone() Attribute {
	return &ShorthandAttribute{
	attribute: sa.attribute,
	Style: sa.Style.Clone().(*ShorthandStyle),
	ID: sa.ID.Clone().(*ShorthandID),
	Roles: make([]*ShorthandRole, len(sa.Roles)),
	Options: make([]*ShorthandOption, len(sa.Options)),
	}
	}
	func (sa *ShorthandAttribute) Clone() Attribute {
	cloned := &ShorthandAttribute{
	attribute: sa.attribute,
	Style: sa.Style.Clone().(*ShorthandStyle),
	ID: sa.ID.Clone().(*ShorthandID),
	Roles: make([]*ShorthandRole, len(sa.Roles)),
	Options: make([]*ShorthandOption, len(sa.Options)),
	}
	for i, r := range sa.Roles {
	cloned.Roles[i] = r.Clone().(*ShorthandRole)
	}
	for i, o := range sa.Options {
	cloned.Options[i] = o.Clone().(*ShorthandOption)
	}
	return cloned
	}

[gemini-code-assist]

The Children() method returns aa.ID, while the Append() method modifies aa.Label. This is inconsistent. The Parent interface, which Children() is part of, implies that Children() should return the elements that can be modified by Append(). To maintain consistency, Children() should probably return aa.Label.

Suggested change

	func (aa *AnchorAttribute) Children() Elements {
	return aa.ID
	}
	func (aa *AnchorAttribute) Children() Elements {
	return aa.Label
	}

[gemini-code-assist]

The Traverse method for Anchor only traverses the ID field and omits the Elements field, which holds the label. This is inconsistent with other parts of the code, such as Equals and NewAnchor, where the label is also considered part of the content. To ensure the entire content of an Anchor is processed during traversal, the Elements field should also be yielded.

Suggested change

	func (aa *Anchor) Traverse(parent Parent) iter.Seq2[Parent, Parent] {
	return func(yield func(Parent, Parent) bool) {
	if !yield(parent, &aa.ID) {
	return
	}
	}
	}
	func (aa *Anchor) Traverse(parent Parent) iter.Seq2[Parent, Parent] {
	return func(yield func(Parent, Parent) bool) {
	if !yield(parent, &aa.ID) {
	return
	}
	if !yield(parent, &aa.Elements) {
	return
	}
	}
	}

[gemini-code-assist]

The elementOverlay is created directly here, but the action field is not set. This will cause the Replace() method on overlayAction to return false, and the replacement will not be applied by the Reader. The replace() helper function should be used instead to ensure the overlayActionReplace flag is set correctly.

Suggested change

	pps.state.overlays[el] = &elementOverlay{replace: el.Elements}
	pps.state.overlays.replace(el, el.Elements)