Skip to content
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

Include original text in annotation location text attribute #6808

Merged

Conversation

anderseknert
Copy link
Member

Previously it would just say '# METADATA'

Fixes #6779

Previously it would just say '# METADATA'

Fixes open-policy-agent#6779

Signed-off-by: Anders Eknert <anders@styra.com>
Comment on lines +2381 to +2387
lines := bytes.Split(b.buf.Bytes(), []byte{'\n'})

for _, line := range lines[:len(lines)-1] {
sb.WriteString("# ")
sb.Write(line)
sb.WriteByte('\n')
}
Copy link
Contributor

@srenatus srenatus Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not the same as replacing every "\n" with "\n# "? like, strings.ReplaceAll? Extra care needs to be applied for the last one... maybe it's a useless optimization 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I hadn't considered that. Will do for next time, but feels like both are OK in this location.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also care would need to be applied for the first line, as it'll need a `# ' too, but isn't preceeded by a newline.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but while it's likely faster, I'm not so sure about readability:

	block := []byte("# METADATA\n# ")
	block = append(block, bytes.TrimSuffix(
		bytes.ReplaceAll(b.buf.Bytes(), []byte{'\n'}, []byte{'\n', '#', ' '}),
		[]byte{'\n', '#', ' '})...,
	)

	result.Location.Text = block

which one do you prefer?

@anderseknert anderseknert merged commit 8f25aaf into open-policy-agent:main Jun 11, 2024
28 checks passed
@anderseknert anderseknert deleted the annotations-location-text branch June 11, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AST: text element in location for annotations just says #METADATA
2 participants