docs-issue-generation: Script can now handle multiple release notes #85490
Conversation
|
This change is |
There was a problem hiding this comment.
Cool. The core intention of what you want the change to do looks good. Here are some comments for improvements.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nickvigilante)
-- commits line 2 at r1:
A couple comments on this line. They also apply to the PR title and body:
- In the cockroach repo, the first line format is "area-of-code: message".
- The reference to the issue the commit is related to should be placed in the body of the message. It should use one of the standard GitHub issue reference verbs or one of
See also:orInforms:. - When referencing a related issue, prefer the GitHub version of the issue if there is one. For this commit, it should be referencing docs-issue-generation: docs issues are not being created for commits with multiple release notes #84434 .
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 28 at r1 (raw file):
// commit contains details about each formatted commit type issue struct { sha string
Suggestion:
sourceCommitSha string
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 30 at r1 (raw file):
sha string title string releaseNote string
Suggestion:
body string
Suggesting this because issue is in the name of the struct. With this change, referencing the field is docsIssue.body instead of docsIssue.issueBody. This is a example of following the DRY principle.
You might also apply this logic to issueTitle above.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 135 at r1 (raw file):
} // getIssues takes a list of commits from GitHub as well as the PR associated with those commits and outputs a formatted list of commits with valid release notes on that PR
This comment also needs to be updated to reflect the new output of the function.
It's also a really long comment. Maybe limit its length to a shorter length? When lines are this long it makes it harder to work on just a laptop screen or have multiple columns of code next to each other on the same screen. It also makes side-by-side code review harder when long lines wrap and break the visual indentation flow.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 26 at r2 (raw file):
) // docsIssue contains details about each formatted commit to be committed to the docs repo
Suggestion:
// docsIssue contains details about each formatted commit to be committed to the docs repo
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 41 at r2 (raw file):
// ghSearch contains a list of search items (in this case, cockroach PRs) from the GitHub API. // This struct holds the search results of all PRs based on a given commit.
Is this line redundant to the line above it? Mabye combine them somehow?
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 49 at r2 (raw file):
// including its number, from the GitHub API. type ghSearchItem struct { Number int `json:"number"` // PR number
Maybe a better name for the field is PRNumber?
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 58 at r2 (raw file):
} // this struct holds the base branch for a cockroach PR from the GitHub API
Could you change this comment to follow the standard go doc commenting format for types?
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 67 at r2 (raw file):
// ghPullCommit holds the SHA and commit message for a particular commit, from the GitHub API type ghPullCommit struct { CRDBSha string `json:"crdbSha"`
Why does this change need to happen?
The JSON coming in from GitHub will use the field name sha, so the annotation json:"sha" needs to stay as it was.
And the variable name doesn't need to explicitly say CRDB. The data this struct will hold could be from any PR (it doesn't logically need to be from a cockroach PR). The context of how this is used doesn't need to be explicit that it's a cockroach sha.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 68 at r2 (raw file):
type ghPullCommit struct { CRDBSha string `json:"crdbSha"` CRDBCommitMsg ghPullCommitMsg `json:"commit"`
Ditto re the name of the variable not needing to change and already being descriptive enough.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 82 at r2 (raw file):
} // the heart of the script to fetch and manipulate all data and create the individual docs docsIssues
There are a lot of places in the comments and strings where issue changed to docsIssue and it doesn't seem like what should have happened (it's like a replace all was run and it found places that shouldn't have changed). Can you double check this was meant everywhere?
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 142 at r2 (raw file):
for _, c := range pullCommit { message := c.CRDBCommitMsg.Message rns := formatReleaseNotes(message, prNumber, c.CRDBSha) // return a slice of data that locates every instance of the phrase "release note (", case insensitive
Could the end-of-line comments be moved before the lines they are for in this function too?
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 158 at r2 (raw file):
func formatReleaseNotes(message string, prNumber int, crdbSha string) []string { // formatReleaseNotes generates a list of docsIssue bodies for the docs repo
This comment should be before the function as the function doc comment. It's describing what the function does.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 161 at r2 (raw file):
// based on a given crdbSha rnBodySlice := []string{} noRN := regexp.MustCompile(`[rR]elease [nN]ote: [nN]one`)
Since the regexs compiled in this function don't change and this function is called many times, it would be good to move them out to be vars in the module like in this example. Then they will be compiled only once and used many times.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 162 at r2 (raw file):
rnBodySlice := []string{} noRN := regexp.MustCompile(`[rR]elease [nN]ote: [nN]one`) if !noRN.MatchString(message) {
How about inverting the logic in this if statement and then out-denting the body of it? This will keep the level of indents in the function lower and make readability better, especially when there is a long body to the block and there is no else at the end.
if noRN.MatchString(message) {
return rnBodySlice
}
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 167 at r2 (raw file):
bugFixRNRE := regexp.MustCompile(`([rR]elease [nN]ote \(bug fix\):.*)`) releaseJustificationRE := regexp.MustCompile(`[rR]elease [jJ]ustification:.*`) buffer := ""
Two things here:
-
Maybe name this
releaseNoteLines(or similar) to indicate what the variable contains? -
I recommend building the release note string using a slice instead of successively concatenating strings together. See method 3 on this page.
When using the + operator to join two strings, an entirely new string is created for every + operator that gets processed (which means the contents of the two strings are copied into a new memory location for the new string). For a 10 line release note, this means it would create 20 different strings (because of the newlines being appended too) with each one being longer than the previous contents of the variable.
Using a slice to concatenate many strings together allows for an efficient creation of the final string by only copying each source string to the final string once.
This problem and solution apply to many (most?) other languages too (JavaScript, Python, Ruby, Java, ...).
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 174 at r2 (raw file):
releaseJustification := releaseJustificationRE.MatchString(x) if bufferNotEmpty && (validRn || releaseJustification) { rnBodySlice = append(rnBodySlice, strings.TrimSuffix(fmt.Sprintf(
I'd recommend generation of the new release note body into a variable before adding it to the rnBodySlice. That will make what is happening much more clear and the indentation will be easier to handle.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 175 at r2 (raw file):
if bufferNotEmpty && (validRn || releaseJustification) { rnBodySlice = append(rnBodySlice, strings.TrimSuffix(fmt.Sprintf( "Related PR: https://github.com/cockroachdb/cockroach/pull/%s\nCommit: https://github.com/cockroachdb/cockroach/commit/%s\n\n---\n\n%s",
Suggestion:
"Related PR: https://github.com/cockroachdb/cockroach/pull/%s\n" +
"Commit: https://github.com/cockroachdb/cockroach/commit/%s\n" +
"\n---\n\n%s",
This will make it much easier to understand what the resulting text will look like when the string is rendered in the issue.
Also, using the + operator in cases where there are a harde-coded strings together like this is OK. It's just when the string being generated will be of unknown length with lots of concatenations that it can become a performance problem.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 190 at r2 (raw file):
} if buffer != "" { // commit whatever is left in the buffer to the rnBodySlice set rnBodySlice = append(rnBodySlice, strings.TrimSuffix(fmt.Sprintf(
Ditto re: generating the new release note body into a variable before adding it to the slice.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 211 at r2 (raw file):
result = message } result = fmt.Sprintf("PR #%d - %s",
Two things:
- It looks like
resultwill always be overwritten here. So whatever was set in theif...elseabove will be lost. - Statements like this are usually be done on one line. It's short and clear what is happening.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 229 at r2 (raw file):
for _, issue := range p.docsIssues { reqBody, err := json.Marshal(map[string]interface{}{ "issueTitle": issue.issueTitle,
This should be:
"title": issue.issueTitle,
pkg/cmd/docs-issue-generation/main.go line 12 at r2 (raw file):
// Check that GitHub PR descriptions and commit messages contain the // expected epic and docsIssue references.
This file also has some strings that look like they've been updated by mistake.
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 28 at r1 (raw file):
Previously, jlinder (James H. Linder) wrote…
Suggestion:
sourceCommitSha string
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 30 at r1 (raw file):
Previously, jlinder (James H. Linder) wrote…
Suggestion:
body stringSuggesting this because
issueis in the name of the struct. With this change, referencing the field isdocsIssue.bodyinstead ofdocsIssue.issueBody. This is a example of following the DRY principle.You might also apply this logic to
issueTitleabove.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 135 at r1 (raw file):
Previously, jlinder (James H. Linder) wrote…
This comment also needs to be updated to reflect the new output of the function.
It's also a really long comment. Maybe limit its length to a shorter length? When lines are this long it makes it harder to work on just a laptop screen or have multiple columns of code next to each other on the same screen. It also makes side-by-side code review harder when long lines wrap and break the visual indentation flow.
Understood. I've split the comment across multiple lines. Also, I moved some of the function calls within this file to have their parameters exist on their own lines.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 26 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Suggestion:
// docsIssue contains details about each formatted commit to be committed to the docs repo
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 41 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Is this line redundant to the line above it? Mabye combine them somehow?
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 49 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Maybe a better name for the field is
PRNumber?
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 58 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Could you change this comment to follow the standard go doc commenting format for types?
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 67 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Why does this change need to happen?
The JSON coming in from GitHub will use the field name
sha, so the annotationjson:"sha"needs to stay as it was.And the variable name doesn't need to explicitly say
CRDB. The data this struct will hold could be from any PR (it doesn't logically need to be from a cockroach PR). The context of how this is used doesn't need to be explicit that it's a cockroach sha.
Accidentally got renamed during my refactor in Goland. Reverted/done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 68 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Ditto re the name of the variable not needing to change and already being descriptive enough.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 82 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
There are a lot of places in the comments and strings where
issuechanged todocsIssueand it doesn't seem like what should have happened (it's like a replace all was run and it found places that shouldn't have changed). Can you double check this was meant everywhere?
Caused by the refactor in Goland. Fixed.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 142 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Could the end-of-line comments be moved before the lines they are for in this function too?
Yes. I did this in other places that had lines longer than 115 characters.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 158 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
This comment should be before the function as the function doc comment. It's describing what the function does.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 161 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Since the regexs compiled in this function don't change and this function is called many times, it would be good to move them out to be vars in the module like in this example. Then they will be compiled only once and used many times.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 162 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
How about inverting the logic in this
ifstatement and then out-denting the body of it? This will keep the level of indents in the function lower and make readability better, especially when there is a long body to the block and there is noelseat the end.if noRN.MatchString(message) { return rnBodySlice }
I think I understand what you're asking here, and I agree. Hoping that this takes care of it.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 167 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Two things here:
Maybe name this
releaseNoteLines(or similar) to indicate what the variable contains?I recommend building the release note string using a slice instead of successively concatenating strings together. See method 3 on this page.
When using the
+operator to join two strings, an entirely new string is created for every+operator that gets processed (which means the contents of the two strings are copied into a new memory location for the new string). For a 10 line release note, this means it would create 20 different strings (because of the newlines being appended too) with each one being longer than the previous contents of the variable.Using a slice to concatenate many strings together allows for an efficient creation of the final string by only copying each source string to the final string once.
This problem and solution apply to many (most?) other languages too (JavaScript, Python, Ruby, Java, ...).
releaseNoteLines is now a string slice. I also use append() to add a new line to releaseNoteLines. I hope that's okay.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 174 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
I'd recommend generation of the new release note body into a variable before adding it to the
rnBodySlice. That will make what is happening much more clear and the indentation will be easier to handle.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 175 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Suggestion:
"Related PR: https://github.com/cockroachdb/cockroach/pull/%s\n" + "Commit: https://github.com/cockroachdb/cockroach/commit/%s\n" + "\n---\n\n%s",This will make it much easier to understand what the resulting text will look like when the string is rendered in the issue.
Also, using the
+operator in cases where there are a harde-coded strings together like this is OK. It's just when the string being generated will be of unknown length with lots of concatenations that it can become a performance problem.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 190 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Ditto re: generating the new release note body into a variable before adding it to the slice.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 211 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Two things:
- It looks like
resultwill always be overwritten here. So whatever was set in theif...elseabove will be lost.- Statements like this are usually be done on one line. It's short and clear what is happening.
I added an intermediate variable called commitTitle to hold the commit title here.
Is it an antipattern if I overwrite a variable with an expression containing itself?
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 229 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
This should be:
"title": issue.issueTitle,
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/docs-issue-generation/main.go line 12 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
This file also has some strings that look like they've been updated by mistake.
Done.
nickvigilante
changed the title
There was a problem hiding this comment.
Great! I've a few more comments.
Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status:
-- commits line 14 at r4:
The release justification can just be:
Release justification: non-production code change
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 135 at r1 (raw file):
Previously, nickvigilante (Nick Vigilante) wrote…
Understood. I've split the comment across multiple lines. Also, I moved some of the function calls within this file to have their parameters exist on their own lines.
Cool. 👍 to the formatting change for long function calls. That makes them much easier to understand.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 162 at r2 (raw file):
Previously, nickvigilante (Nick Vigilante) wrote…
I think I understand what you're asking here, and I agree. Hoping that this takes care of it.
Excellent. Yes, you got what I was asking. Thanks!
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 167 at r2 (raw file):
Previously, nickvigilante (Nick Vigilante) wrote…
releaseNoteLinesis now a string slice. I also useappend()to add a new line toreleaseNoteLines. I hope that's okay.
Great! Yes, using append() here is appropriate and expected.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 211 at r2 (raw file):
Previously, nickvigilante (Nick Vigilante) wrote…
I added an intermediate variable called
commitTitleto hold the commit title here.Is it an antipattern if I overwrite a variable with an expression containing itself?
Ah! I think I missed that result was used in the fmt.Sprintf call.
There are certainly times when overwriting the variable like you were doing with result is appropriate. In this case, using commitTitle as the initial variable makes this code much clearer because it commitTitle variable now states specifically what it represents. If it had that name originally, I don't think I would have left the comment.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 91 at r4 (raw file):
params.Token, &search, ); err != nil {
I'd suggest separating out the call to httpGet() into a separate line like this:
err := httpGet(
"https://api.github.com/search/issues?q=sha:"+params.Sha+"+repo:cockroachdb/cockroach+is:merged",
params.Token,
&search,
)
if err != nil {
Putting the very long call to httpGet() within the if statement makes it harder to understand the if statement. And since most people who interact with this code will be reading it and trying to understand it after this PR is merged, it's much better to keep the code simpler to understand.
Code quote:
if err := httpGet(
"https://api.github.com/search/issues?q=sha:"+params.Sha+"+repo:cockroachdb/cockroach+is:merged",
params.Token,
&search,
); err != nil {pkg/cmd/docs-issue-generation/docs_issue_generation.go line 97 at r4 (raw file):
for _, pr := range prs { // for each PR in the list, get the merge branch and the list of eligible commits var pull ghPull if err := httpGet(
Ditto here on separating out the call to httpGet() into a separate line.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 106 at r4 (raw file):
pr.mergeBranch = pull.Base.Ref var commits []ghPullCommit if err := httpGet(
And ditto here on separating out the call to httpGet() into a separate line.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 176 at r4 (raw file):
var ( noRN = regexp.MustCompile(`[rR]elease [nN]ote: [nN]one`)
Since the other variable names for regexps all end with RE, how about making this one consistent with them?
noRNRE = regexp.MustCompile(`[rR]elease [nN]ote: [nN]one`)
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 193 at r4 (raw file):
I'd suggest inserting len(releaseNoteLines) > 0 directly into the if statements where bufferNotEmpty is used. The biggest reason is because possible side effects if the contents of releaseNoteLines change between the two uses of bufferNotEmpty. Using a pre-calculated value based on a variable that can change between the uses of the pre-calculated value in two if statements that are somewhat complex on their own is complicated. It's better to simplify and directly code in the calculation in both places.
I go back to these two ideas from the Zen of Python all the time. I think they apply here well:
Simple is better than complex.
Complex is better than complicated.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 198 at r4 (raw file):
// if the buffer isn't empty and the next line is either a release justification or a new RN, write the buffer // to rnBodySlice and flush the buffer if bufferNotEmpty && (validRn || releaseJustification) {
Does the comment for this if block state the same thing as the code itself states? If the words in the comment are more descriptive than the variable names, would it be better to change the names of the variables?
If you do choose to keep the comment, maybe update it to use language related to "release note lines" instead of the old "buffer" related language?
Code quote:
// if the buffer isn't empty and the next line is either a release justification or a new RN, write the buffer
// to rnBodySlice and flush the buffer
if bufferNotEmpty && (validRn || releaseJustification) {pkg/cmd/docs-issue-generation/docs_issue_generation.go line 244 at r4 (raw file):
func (p pr) createDocsIssues(token string) { postURL := "https://api.github.com/repos/cockroachdb/docs/docsIssues"
It looks like this URL was hit by the search and replace too?
postURL := "https://api.github.com/repos/cockroachdb/docs/issues"
Previously, if a commit to the cockroach repo contained multiple RNs, the docs issue generation script only evaluated the last release note. Additionally, if the release justification was included below the last release note in a commit, it would be included as part of the release note. Now, each release note in a commit is evaluated, and the release justification is stripped. Release note: None Release justification: non-production code change
There was a problem hiding this comment.
Reviewable status:
Previously, jlinder (James H. Linder) wrote…
A couple comments on this line. They also apply to the PR title and body:
- In the cockroach repo, the first line format is "area-of-code: message".
- The reference to the issue the commit is related to should be placed in the body of the message. It should use one of the standard GitHub issue reference verbs or one of
See also:orInforms:.- When referencing a related issue, prefer the GitHub version of the issue if there is one. For this commit, it should be referencing docs-issue-generation: docs issues are not being created for commits with multiple release notes #84434 .
Done.
Previously, jlinder (James H. Linder) wrote…
The release justification can just be:
Release justification: non-production code change
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 135 at r1 (raw file):
Previously, jlinder (James H. Linder) wrote…
Cool. 👍 to the formatting change for long function calls. That makes them much easier to understand.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 162 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Excellent. Yes, you got what I was asking. Thanks!
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 167 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Great! Yes, using
append()here is appropriate and expected.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 211 at r2 (raw file):
Previously, jlinder (James H. Linder) wrote…
Ah! I think I missed that
resultwas used in thefmt.Sprintfcall.There are certainly times when overwriting the variable like you were doing with
resultis appropriate. In this case, usingcommitTitleas the initial variable makes this code much clearer because itcommitTitlevariable now states specifically what it represents. If it had that name originally, I don't think I would have left the comment.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 91 at r4 (raw file):
Previously, jlinder (James H. Linder) wrote…
I'd suggest separating out the call to
httpGet()into a separate line like this:err := httpGet( "https://api.github.com/search/issues?q=sha:"+params.Sha+"+repo:cockroachdb/cockroach+is:merged", params.Token, &search, ) if err != nil {Putting the very long call to
httpGet()within theifstatement makes it harder to understand the if statement. And since most people who interact with this code will be reading it and trying to understand it after this PR is merged, it's much better to keep the code simpler to understand.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 97 at r4 (raw file):
Previously, jlinder (James H. Linder) wrote…
Ditto here on separating out the call to
httpGet()into a separate line.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 106 at r4 (raw file):
Previously, jlinder (James H. Linder) wrote…
And ditto here on separating out the call to
httpGet()into a separate line.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 176 at r4 (raw file):
Previously, jlinder (James H. Linder) wrote…
Since the other variable names for regexps all end with
RE, how about making this one consistent with them?noRNRE = regexp.MustCompile(`[rR]elease [nN]ote: [nN]one`)
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 193 at r4 (raw file):
Previously, jlinder (James H. Linder) wrote…
I'd suggest inserting
len(releaseNoteLines) > 0directly into theifstatements wherebufferNotEmptyis used. The biggest reason is because possible side effects if the contents ofreleaseNoteLineschange between the two uses ofbufferNotEmpty. Using a pre-calculated value based on a variable that can change between the uses of the pre-calculated value in twoifstatements that are somewhat complex on their own is complicated. It's better to simplify and directly code in the calculation in both places.I go back to these two ideas from the Zen of Python all the time. I think they apply here well:
Simple is better than complex.
Complex is better than complicated.
Done.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 198 at r4 (raw file):
Previously, jlinder (James H. Linder) wrote…
Does the comment for this
ifblock state the same thing as the code itself states? If the words in the comment are more descriptive than the variable names, would it be better to change the names of the variables?If you do choose to keep the comment, maybe update it to use language related to "release note lines" instead of the old "buffer" related language?
I just omitted the comment as it largely does just restate what the code does.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 244 at r4 (raw file):
Previously, jlinder (James H. Linder) wrote…
It looks like this URL was hit by the search and replace too?
postURL := "https://api.github.com/repos/cockroachdb/docs/issues"
Done.
There was a problem hiding this comment.
Excellent!
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:
Previously, nickvigilante (Nick Vigilante) wrote…
Done.
👍
This change could also be applied to the PR body.
There was a problem hiding this comment.
Reviewable status:
Previously, jlinder (James H. Linder) wrote…
👍
This change could also be applied to the PR body.
Done.
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
Previously, if a commit to the cockroach repo contained multiple
RNs, the docs issue generation script only evaluated the
last release note. Additionally, if the release justification was
included below the last release note in a commit, it would be
included as part of the release note. Now, each release note in a
commit is evaluated, and the release justification is stripped.
Fixes #84434
Release note: None
Release justification: non-production code change