Skip to content

Document SanitizedAnchorName algorithm, copy implementation. #352

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

Merged
merged 2 commits into from
May 9, 2017

Conversation

dmitshur
Copy link
Collaborator

@dmitshur dmitshur commented May 2, 2017

The goal of this change is to reduce number of non-standard library packages (repositories) that blackfriday imports from 1 to 0, and in turn, reduce the cost of importing blackfriday into other projects.

Do so by documenting the algorithm of SanitizedAnchorName, and include a copy of the small function inside blackfriday itself. The same functionality continues to be available in the original location, github.com/shurcooL/sanitized_anchor_name.Create. It can be used by existing users and those that look for a small package, and don't need all of blackfriday functionality. Existing users of blackfriday can use the new SanitizedAnchorName function directly and avoid an extra package import.

Resolves #350.

The goal of this change is to reduce number of non-standard library
packages (repositories) that blackfriday imports from 1 to 0, and in
turn, reduce the cost of importing blackfriday into other projects.

Do so by documenting the algorithm of SanitizedAnchorName, and include
a copy of the small function inside blackfriday itself. The same
functionality continues to be available in the original location,
github.com/shurcooL/sanitized_anchor_name.Create. It can be used by
existing users and those that look for a small package, and don't need
all of blackfriday functionality. Existing users of blackfriday can use
the new SanitizedAnchorName function directly and avoid an extra
package import.

Resolves #350.
// This algorithm is also implemented in a small standalone package at
// github.com/shurcooL/sanitized_anchor_name. It can be useful for clients
// that want a small package and don't need full functionality of blackfriday.
package blackfriday
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's what it looks like, for reference.

image

@russross
Copy link
Owner

russross commented May 2, 2017

LGTM--Thanks for doing this!

@rtfb
Copy link
Collaborator

rtfb commented May 3, 2017

This is great, thanks! The only complaint that I have is the duplicate paragraphs in README and doc.go. Would it perhaps be better to leave the reference text in doc.go and link to it from the README?

@dmitshur
Copy link
Collaborator Author

dmitshur commented May 3, 2017

The only complaint that I have is the duplicate paragraphs in README and doc.go. Would it perhaps be better to leave the reference text in doc.go and link to it from the README?

I did that based on how the go-github package did it. It has similar documentation in godoc and README:

And we keep them in sync (google/go-github#397).

Personally, I always prefer looking at godoc, but I guess the idea is that beginners to Go may miss that, so putting it in the README helps them catch it.

However, I like your idea of linking to the algorithm specification from the README to the godoc. That way, the specification is in one canonical place and cannot get out of sync.

I'll apply that change.

@dmitshur
Copy link
Collaborator Author

dmitshur commented May 3, 2017

@rtfb, how do you feel about adding an exported version of SanitizedAnchorName to the blackfriday API vs keeping it unexported?

Also, if we do export it, do you have opinions on whether it should be inside blackfriday package itself, or if it can be in a small standalone subpackage (inside blackfriday repository)?

@rtfb
Copy link
Collaborator

rtfb commented May 3, 2017

@rtfb, how do you feel about adding an exported version of SanitizedAnchorName to the blackfriday API vs keeping it unexported?

I guess that depends on how likely are people to have projects which call blackfriday in one place and SanitizedAnchorName in another. I can certainly imagine that, but the likelihood seems small. I don't have strong opinion either way, both seem to be adequate.

Also, if we do export it, do you have opinions on whether it should be inside blackfriday package itself, or if it can be in a small standalone subpackage (inside blackfriday repository)?

I say, inside. Can't foresee that subpackage growing, and one function packages are a bit weird, especially when the function is so specific.

This way, the specification has a canonical location and doesn't need
to be kept in sync between the README and godoc.
Copy link
Collaborator

@rtfb rtfb left a comment

Choose a reason for hiding this comment

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

LGTM

@dmitshur
Copy link
Collaborator Author

dmitshur commented May 3, 2017

@adg, how does this solution for #350 look to you?

@rtfb rtfb mentioned this pull request May 6, 2017
4 tasks
@adg
Copy link

adg commented May 8, 2017

LGTM thanks.

@dmitshur dmitshur merged commit 0ba0f2b into master May 9, 2017
@dmitshur dmitshur deleted the document-and-copy-sanitized_anchor_name branch May 9, 2017 06:07
dmitshur added a commit that referenced this pull request Dec 23, 2018
The goal of this change is to reduce number of non-standard library
packages (repositories) that blackfriday imports (not counting imports
used only for tests) from 1 to 0, and in turn, reduce the cost of
importing blackfriday into other projects.

Do so by documenting the algorithm of SanitizedAnchorName, and include
a copy of the small function inside blackfriday itself. The same
functionality continues to be available in the original location,
github.com/shurcooL/sanitized_anchor_name.Create. It can be used by
existing users and those that look for a small package, and don't need
all of blackfriday functionality. Existing users of blackfriday can use
the new SanitizedAnchorName function directly and avoid an extra
package import.

This change is a port of PR #352 from v1 into v2.

Updates #348.
Updates #350.
dmitshur added a commit that referenced this pull request Jan 20, 2019
The goal of this change is to reduce number of non-standard library
packages (repositories) that blackfriday imports (not counting imports
used only for tests) from 1 to 0, and in turn, reduce the cost of
importing blackfriday into other projects.

Do so by documenting the algorithm of SanitizedAnchorName, and include
a copy of the small function inside blackfriday itself. The same
functionality continues to be available in the original location,
github.com/shurcooL/sanitized_anchor_name.Create. It can be used by
existing users and those that look for a small package, and don't need
all of blackfriday functionality. Existing users of blackfriday can use
the new SanitizedAnchorName function directly and avoid an extra
package import.

This change is a port of PR #352 from v1 into v2.

Updates #348.
Updates #350.
willdollman pushed a commit to willdollman/blackfriday that referenced this pull request Feb 27, 2021
…s#352)

The goal of this change is to reduce number of non-standard library
packages (repositories) that blackfriday imports from 1 to 0, and in
turn, reduce the cost of importing blackfriday into other projects.

Do so by documenting the algorithm of SanitizedAnchorName, and include
a copy of the small function inside blackfriday itself. The same
functionality continues to be available in the original location,
github.com/shurcooL/sanitized_anchor_name.Create. It can be used by
existing users and those that look for a small package, and don't need
all of blackfriday functionality. Existing users of blackfriday can use
the new SanitizedAnchorName function directly and avoid an extra
package import.

Resolves russross#350.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants