Snippet creation and editing on the website. #85
No reviewers
Labels
No Label
admins only
bug
design
duplicate
gimme feedback
good first issue
hmmmm
invalid
reference
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: hmn/hmn#85
Loading…
Reference in New Issue
No description provided.
Delete Branch "snippet_edit"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
@ -467,2 +467,4 @@
adminCommand.AddCommand(moveThreadsToSubforumCommand)
fixupSnippetAssociation := &cobra.Command{
Use: "fixupsnippets",
Will this command be necessary after we we roll this out, or is it a one-time thing?
One time.
@ -0,0 +81,4 @@
logging.ExtractLogger(ctx).Debug().Msg("Fetching embed")
client := &http.Client{
Timeout: timeout,
}
It's recommended that
http.Client
is reused where possible instead of created every time, because the client'sTransport
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.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
}
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 thehttp
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()
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
}
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)
I don't think it really matters, but you could simply use
FindAllSubmatch
onpartialHtml
instead of converting to a string and usingFindAllStringSubmatch
. Everything in theregexp
package hasbyte[]
andstring
equivalents.Also, naming nitpick, I think we could name this
metaTags
or something because I got confused and had to look back up to see whatmatches
was actually matching.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 {
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
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;
}
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), ""))
},
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?
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.
ea1e753d6b
toa9b0606b79
Pull request closed