copy sanitized_anchor_name to blackfriday

[adg]

The blackfriday package only has one external dependency: github.com/shurcooL/sanitized_anchor_name

That repo provides a single ~20-line function. Can we copy it into this repository to remove the external dependency?

cc @shurcooL

[dmitshur]

Previous discussion is at #139. The reason it's in a separate repo is described in #139 (comment).

But we can reconsider this issue. I'm going to think about it.

[dmitshur]

@adg Can you elaborate on the motivation to do this? I can certainly guess, but it'd be helpful if you made it explicit. What problem are we trying to resolve, exactly?

[adg]

Sorry that I missed #139.

Dependencies have costs. They introduce fragility and complexity. When adding a dependency on another repository, we must weigh up whether that cost is justified. I don't believe it is, in this case. (Ultimately the choice is subjective, though.)

Take a look at the https://github.com/upspin/upspin repository, for example. I just completed a bunch of work to move our cloud-related dependencies out of the core repo. The remaining external dependencies are:

bazil.org/fuse
bazil.org/fuse/fs
bazil.org/fuse/fuseutil
github.com/golang/protobuf/proto
github.com/russross/blackfriday
github.com/shurcooL/sanitized_anchor_name
golang.org/x/crypto/acme
golang.org/x/crypto/acme/autocert
golang.org/x/crypto/hkdf
golang.org/x/net/context
golang.org/x/net/context/ctxhttp
golang.org/x/text/cases
golang.org/x/text/internal
golang.org/x/text/internal/tag
golang.org/x/text/language
golang.org/x/text/runes
golang.org/x/text/secure/bidirule
golang.org/x/text/secure/precis
golang.org/x/text/transform
golang.org/x/text/unicode/bidi
golang.org/x/text/unicode/norm
golang.org/x/text/width
gopkg.in/yaml.v2

Looking at each repo, you can see why the dependency is justified. Each of fuse, proto, crypto, net, text, and yaml provide substantial and essential functionality required to make our system work. The sanitized_anchor_name is a tiny piece of functionality, IMO undeserving of its own repository (in this context).

I suggest that blackfriday either copy this function or vendor the repository. The former is preferable to the latter.

[russross]

I support copying the function. I think that one of blackfriday's virtues has been that it is very boring, and having zero dependencies outside the standard library helps to keep it boring. It gives users one less thing to worry about.

[dmitshur]

@adg Thanks, that's helpful.

Dependencies have costs. They introduce fragility and complexity. When adding a dependency on another repository, we must weigh up whether that cost is justified.

So, if we can avoid having the sanitized_anchor_name dependency in blackfriday, the cost of importing blackfriday into people's projects would be lowered.

The sanitized_anchor_name is a tiny piece of functionality, IMO undeserving of its own repository (in this context).

I agree. If we can find a good way to make it no longer be a separate repository, that would be improvement (although it has a cost of making the change, but likely worth it).

I suggest that blackfriday either copy this function or vendor the repository. The former is preferable to the latter.

I'd really like to avoid making a copy of the function, because that would go against its purpose. The point of sanitized_anchor_name.Create is that it's guaranteed to generate a matching id for a given text. Users of that functionality can be sure it won't ever go out of sync with the logic that blackfriday uses, and maintainers of blackfriday won't have to remember to update 2 pieces of code.

However, we can move the package into the blackfriday repository. It'd still be a separate package, but no longer a separate repository, so the original goal of reducing the cost of importing blackfriday would be satisfied (by some positive amount).

By moving the package, I mean we'd create a copy inside blackfriday repository, and deprecate the current github.com/shurcooL/sanitized_anchor_name.

Users of sanitized_anchor_name

Looking at the current known importers of github.com/shurcooL/sanitized_anchor_name, there aren't many. If we filter out all the forks of blackfriday itself, it leaves:

github.com/9uuso/vertigo/databases/sqlx         Package sqlx is PostgreSQL and SQLite database access layer (DAL) for users and posts.
github.com/chaseadamsio/goorgeous               A go org syntax parser to html
github.com/shurcooL/frontend/table-of-contents
github.com/shurcooL/github_flavored_markdown    Package github_flavored_markdown provides a GitHub Flavored Markdown renderer with fenced code block highlighting, clickable heading anchor links.
github.com/shurcooL/home/idiomaticgo            Package idiomaticgo contains functionality for rendering /idiomatic-go page.
github.com/shurcooL/play/63
github.com/shurcooL/play/74

I've looked over them, and pretty much each user of sanitized_anchor_name also imports blackfriday (sometimes indirectly):

• sqlx uses sanitized_anchor_name in two places to create post.Slug from post.Title, and it imports blackfriday in the same .go file:

  "github.com/russross/blackfriday"
  slug "github.com/shurcooL/sanitized_anchor_name"

• goorgeous uses sanitized_anchor_name to create headlineID in a generateHeadline method. It imports both sanitized_anchor_name and blackfriday right next to each other as well.

• github.com/shurcooL/frontend/table-of-contents doesn't use blackfriday directly, but it's meant to be used in a context where blackfriday is used anyway.

• github.com/shurcooL/github_flavored_markdown is a blackfriday.Renderer implementation, it therefore uses blackfriday.

• github.com/shurcooL/home/idiomaticgo uses sanitized_anchor_name to create anchors matching output of github_flavored_markdown, which in turn depends on blackfriday.

• github.com/shurcooL/play/... are just one-off experimental snippets and don't matter.

So, given all/most users of sanitized_anchor_name package also import blackfriday package, it shouldn't be a big cost for them if we move the package from github.com/shurcooL/sanitized_anchor_name to something like github.com/russross/blackfriday/sanitized_anchor_name. I can even make PRs to update them.

Proposal

Based on the above, my proposal is the following:

• Move github.com/shurcooL/sanitized_anchor_name package from a standalone repository to blackfriday repository, making its import path something like github.com/russross/blackfriday/sanitized_anchor_name.
• Deprecate old copy of github.com/shurcooL/sanitized_anchor_name, remove it after some time, after its users have had a chance to migrate.

This will not be a breaking change for blackfriday users, but it will be for sanitized_anchor_name users. But there aren't that many of them.

• Since it's a breaking change, we can use the opportunity to finally remove the unidiomatic underscores from the import path (the package is so old, it predates me knowing how to name Go packages well):

  github.com/russross/blackfriday/sanitizedanchorname

  It might be better to give it an even shorter name. I'm open to suggestions.

How does that sound?

[adg]

Here's my counterproposal:

• Document the algorithm that sanitized_anchor_name uses in the package docs, so that anyone can implement a compatible version (and also so that you are committing to the algorithm),
• Copy the function to blackfriday/block.go, unexported, and document that it implements the algorithm of sanitized_anchor_name.

Rationale:

• Doesn't break existing users.
• Nobody should be importing blackfriday/sanitizedanchorname for anything. If people want to import it from your tiny repo, then that's fine. I think it's inappropriate for blackfriday to import it from there, but I don't think it's generally inappropriate.
• The algorithm used to generate the anchors is more important than the specific code that does it, and it shouldn't ever change to keep things interoperable.

[adg]

One tweak to the above: for users like sqlx it might be appropriate to export the function.

[dmitshur]

I was just thinking that the reason for all this difficulty in finding a solution without compromises is that there's no specification, and as a result, the implementation is the only specification itself.

If there was a specification for this, then it's as you say, the current sanitized_anchor_name can be a small package that implements only that algorithm. blackfriday can implement the same spec in its internal code, given the overhead of importing a package in a separate repository wouldn't be worth it.

Where should this specification exist, and is there anyone who'd want to take on creating it? Edit: I missed that you said "in the package docs". That seems like a reasonable place.

Somewhat related to this, when GitHub announced they're releasing a formal specification for GitHub Flavored Markdown, I was really hoping that would include a specification for escaping heading anchor names that GFM uses. That would help resolve issues such as avelino/awesome-go#1224 (comment), and we could just use that same spec instead of creating our own. But, I didn't find it, and I tried asking without luck.

One tweak to the above: for users like sqlx it might be appropriate to export the function.

Can you clarify what you mean by that? Export what function, and where?

[adg]

Can you clarify what you mean by that? Export what function, and where?

blackfriday could export the sanitization function so that sqlx, which presumably uses both blackfriday and sanitized_anchor_name in concert, would only need to import blackfriday.

Independent of the future plans for sanitized_anchor_name, I support @russross in copying the function.

[dmitshur]

Independent of the future plans for sanitized_anchor_name, I support @russross in copying the function.

I'll just say this, I don't like that option because it creates the burden of maintaining and keeping sanitized_anchor_name in sync with blackfriday's internal copy, and I would feel most responsible for taking that burden on (because I'm not sure if others would, and the import path has my username in it).

However, I think I'm okay with it, because it probably benefits more people than it inconveniences.

Also, there are two things make the burden quite small. First, the algorithm hasn't changed in the last 3 years, and I don't expect it'll change too often in the future. Second, I'm helping maintain this repository (although less actively lately; @rtfb is doing most of the work these days, especially on the v2 front) and I expect it'll be relatively easy for me to be made aware of any changes that I'd need to update sanitized_anchor_name for. We can also add a comment // Keep this in sync with ... to the copied function, to help a bit.

/cc @rtfb Do you have thoughts on this issue?

[dmitshur]

I took a stab at resolving this issue in PR #352. It's just a first draft, I welcome feedback and suggestions. @adg, can you take a look and see what you think?

One part I was hesitating about was whether to make blackfriday.SanitizedAnchorName exported or not exported. I went with exported for now, per your tweak in #350 (comment). But still unsure about this.