Snippet creation and editing on the website. #85

Closed
AsafGartner wants to merge 0 commits from snippet_edit into jam-2022
Owner
No description provided.
AsafGartner added 2 commits 2022-08-05 04:21:22 +00:00
bvisness requested changes 2022-08-05 05:03:20 +00:00
@ -467,2 +467,4 @@
adminCommand.AddCommand(moveThreadsToSubforumCommand)
fixupSnippetAssociation := &cobra.Command{
Use: "fixupsnippets",
Owner

Will this command be necessary after we we roll this out, or is it a one-time thing?

Will this command be necessary after we we roll this out, or is it a one-time thing?
Author
Owner

One time.

One time.
@ -0,0 +81,4 @@
logging.ExtractLogger(ctx).Debug().Msg("Fetching embed")
client := &http.Client{
Timeout: timeout,
}
Owner

It's recommended that http.Client is reused where possible instead of created every time, because the client's Transport has an internal cache that is good to keep around. (See the docs.) So I think this should be moved to a package-level variable.

It's recommended that `http.Client` is reused where possible instead of created every time, because the client's `Transport` has an internal cache that is good to keep around. (See [the docs](https://pkg.go.dev/net/http#Client).) So I think this should be moved to a package-level variable.
Author
Owner

Since I'm setting the timeout from the function argument, I don't think it would be a good idea to have the client on the package level, but I bumped it one function up so that it can be used for multiple urls.

Since I'm setting the timeout from the function argument, I don't think it would be a good idea to have the client on the package level, but I bumped it one function up so that it can be used for multiple urls.
@ -0,0 +89,4 @@
res, err := client.Do(req)
if err != nil {
return nil, err
}
Owner

You should defer res.Body.Close() right after this so you don't have to worry about it elsewhere. (This is what they do in the http package docs.)

You should `defer res.Body.Close()` right after this so you don't have to worry about it elsewhere. (This is what they do in the `http` package docs.)
@ -0,0 +98,4 @@
if strings.HasPrefix(contentType, "text/html") || strings.HasPrefix(contentType, "application/html") {
var buffer bytes.Buffer
_, err := io.CopyN(&buffer, res.Body, 100*1024) // NOTE(asaf): If the opengraph stuff isn't in the first 100kb, we don't care.
res.Body.Close()
Owner

No need for this or any other res.Body.Close() if you defer above like is recommended.

No need for this or any other `res.Body.Close()` if you defer above like is recommended.
@ -0,0 +116,4 @@
res, err = client.Do(req)
if err != nil {
return nil, err
}
Owner

Need to defer res.Body.Close() on this too.

Need to `defer res.Body.Close()` on this too.
@ -0,0 +171,4 @@
keyIdx := metaAttrRegex.SubexpIndex("key")
valueIdx := metaAttrRegex.SubexpIndex("value")
html := string(partialHtml)
matches := metaRegex.FindAllStringSubmatch(html, -1)
Owner

I don't think it really matters, but you could simply use FindAllSubmatch on partialHtml instead of converting to a string and using FindAllStringSubmatch. Everything in the regexp package has byte[] and string equivalents.

I don't think it really matters, but you could simply use `FindAllSubmatch` on `partialHtml` instead of converting to a string and using `FindAllStringSubmatch`. Everything in the `regexp` package has `byte[]` and `string` equivalents.
Owner

Also, naming nitpick, I think we could name this metaTags or something because I got confused and had to look back up to see what matches was actually matching.

Also, naming nitpick, I think we could name this `metaTags` or something because I got confused and had to look back up to see what `matches` was actually matching.
Author
Owner

FindAllSubmatch returns a byte array, so I would need to cast somewhere anyway.

`FindAllSubmatch` returns a byte array, so I would need to cast somewhere anyway.
@ -0,0 +173,4 @@
html := string(partialHtml)
matches := metaRegex.FindAllStringSubmatch(html, -1)
for _, m := range matches {
if len(m) > 1 {
Owner

Is it even possible for this if statement to be false? For each matched meta tag, you should always have a slice with two elements.

Is it even possible for this if statement to be false? For each matched meta tag, you should always have a slice with two elements.
@ -0,0 +191,4 @@
}
}
if content != "" && prop != "" {
return content
Owner

What is prop used for? I'm confused. We don't seem to return it or use it in any way?

What is `prop` used for? I'm confused. We don't seem to return it or use it in any way?
@ -336,0 +359,4 @@
.gap5 {
gap: $spacing-extra-large;
}
Owner

I actually already added gap helpers - g1, g2, etc.

I actually already added gap helpers - `g1`, `g2`, etc.
@ -218,1 +221,4 @@
},
"cleancontrolchars": func(str template.HTML) template.HTML {
return template.HTML(controlCharRegex.ReplaceAllString(string(str), ""))
},
Owner

Gross. I don't have a better idea, but gross.

Although why aren't we just doing this in our plain old Go code? Would we ever want control characters to stick around?

Gross. I don't have a better idea, but gross. Although why aren't we just doing this in our plain old Go code? Would we ever want control characters to stick around?
Author
Owner

It's only an issue with RSS and hand-written JSON (and the only use case for this template function is RSS), so probably not worth handling it on the input side?
If we do, we would also need to clean our db.

It's only an issue with RSS and hand-written JSON (and the only use case for this template function is RSS), so probably not worth handling it on the input side? If we do, we would also need to clean our db.
AsafGartner force-pushed snippet_edit from ea1e753d6b to a9b0606b79 2022-08-05 21:50:58 +00:00 Compare
AsafGartner closed this pull request 2022-08-06 02:22:27 +00:00

Pull request closed

Sign in to join this conversation.
No description provided.