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

Unexport a bunch of constants private to HTML renderer #381

Merged
merged 2 commits into from
Aug 2, 2017

Conversation

rtfb
Copy link
Collaborator

@rtfb rtfb commented Jul 29, 2017

They were cluttering the documentation and are not useful otherwise.

They were cluttering the documentation and are not useful otherwise.
@rtfb rtfb mentioned this pull request Jul 29, 2017
Copy link
Collaborator

@dmitshur dmitshur left a comment

Choose a reason for hiding this comment

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

Are they used anywhere? I'm not seeing any places where they're used being updated as part of this PR.

@rtfb
Copy link
Collaborator Author

rtfb commented Jul 30, 2017

They're only used to initialize the htmlTagRe variable, last line of the diff. That's all they are, a big regexp broken into smaller pieces. The regexp is then used to wipe all html inside <img>.

@dmitshur
Copy link
Collaborator

dmitshur commented Jul 30, 2017

They're only used to initialize the htmlTagRe variable, last line of the diff.

Ah, I see that now, thanks. 👍

FWIW, I think it's more readable in general to define higher level things on top (which use lower level things). Then define the lower level things below them. That way, the reader sees uses of things that aren't defined yet, but they acquire context (e.g., "ok, so htmlTag is something made up from openTag, closeTag, etc.; now let me see what those things are"). Then they can look up the implementation details of the lower level helpers (if they want to learn more details).

This is possible in Go for package scope identifiers, because they can be arranged in any order and the compiler figures out the correct order to initialize them (based on use). It's harder in languages like C++ where things are defined in order they show up in source code, so people more often define lower level things first, higher level ones later (although headers let you get around that limitation... except for #define directives).

So, I think the following arrangement would be more readable:

const (
	htmlTag               = "(?:" + openTag + "|" + closeTag + "|" + htmlComment + "|" +
		processingInstruction + "|" + declaration + "|" + cdata + ")"
	openTag               = "<" + tagName + attribute + "*" + "\\s*/?>"
	closeTag              = "</" + tagName + "\\s*[>]"
	htmlComment           = "<!---->|<!--(?:-?[^>-])(?:-?[^-])*-->"
	processingInstruction = "[<][?].*?[?][>]"
	declaration           = "<![A-Z]+" + "\\s+[^>]*>"
	cdata                 = "<!\\[CDATA\\[[\\s\\S]*?\\]\\]>"
	tagName               = "[A-Za-z][A-Za-z0-9-]*"
	attribute             = "(?:" + "\\s+" + attributeName + attributeValueSpec + "?)"
	attributeName         = "[a-zA-Z_:][a-zA-Z0-9:._-]*"
	attributeValueSpec    = "(?:" + "\\s*=" + "\\s*" + attributeValue + ")"
	attributeValue        = "(?:" + unquotedValue + "|" + singleQuotedValue + "|" + doubleQuotedValue + ")"
	unquotedValue         = "[^\"'=<>`\\x00-\\x20]+"
	singleQuotedValue     = "'[^']*'"
	doubleQuotedValue     = "\"[^\"]*\""
)

This is just an optional style suggestion.

@rtfb
Copy link
Collaborator Author

rtfb commented Aug 1, 2017

Yeah, this reasoning makes perfect sense. I've rearranged the constants in roughly opposite order.

@rtfb rtfb merged commit 4582051 into v2 Aug 2, 2017
@rtfb rtfb deleted the v2-unexport-some-consts branch October 15, 2017 08:19
This pull request was closed.
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