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

Allow link in attribution #806

Merged
merged 1 commit into from
Sep 7, 2021
Merged

Conversation

underspica
Copy link
Contributor

Issue #798
Allow link in attribution by unescape anchor tag.

Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

A couple of minor comments added inline. Please add tests cases. Thanks for the contribution!

newMap.Attribution = html.EscapeString(string(cfg.Attribution))

attribution := html.EscapeString(string(cfg.Attribution))
re := regexp.MustCompile(`<(a\s(.+?)|/a)>`)
Copy link
Member

Choose a reason for hiding this comment

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

Please move the regexp.MustCompile outside the function call.

Comment on lines 17 to 22
attribution := html.EscapeString(string(cfg.Attribution))
re := regexp.MustCompile(`<(a\s(.+?)|/a)>`)
tags := re.FindAllString(attribution, -1)
for _, tag := range tags {
attribution = strings.Replace(attribution, tag, html.UnescapeString(tag), 1)
}
Copy link
Member

Choose a reason for hiding this comment

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

You could move this to a separate function and maybe call it sanitizeAttribution. That would allow you to add your test cases to the codebase which would be ideal.

@underspica
Copy link
Contributor Author

Please review the new commits

@ARolek
Copy link
Member

ARolek commented Aug 25, 2021

@underspica this looks excellent! Thanks for making the adjustments. The last request is to squash everything into a single commit then I will get this merged in. Thank you!

Allow link in attribution
Test SanitizeAttribution
@underspica
Copy link
Contributor Author

Done, thank you.

@ARolek ARolek merged commit 89c6366 into go-spatial:v0.14.x Sep 7, 2021
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.

2 participants