require: fix invalid examples in doc comments (codegen)#1780
require: fix invalid examples in doc comments (codegen)#1780PCloud63514 wants to merge 10 commits intostretchr:masterfrom
Conversation
| return strings.Replace(f.Comment(), search, replace, -1) | ||
| } | ||
|
|
||
| func requireCommentParseIf(s string) string { |
There was a problem hiding this comment.
This method should be tested
There was a problem hiding this comment.
This is codegen. Tests are not strictly necessary: checking the generated output is enough.
However, tests would still be helpful to document what the function expects and does. Tests will also be useful for future refactorings.
Even more helpful would be some godoc for the function because the require prefix is misleading: on a first read I got it as verb while it is about the require package.
There was a problem hiding this comment.
An issue is that it's unclear from the function name or the code; what is this function is supposed to do. A test or a good comment before the function would help.
There was a problem hiding this comment.
That was my point, indeed
There was a problem hiding this comment.
#1780 (comment)
I wasn’t familiar with the reply feature, so I didn’t realize multiple conversations were happening. 😅
Considering tests, would it be better to separate this method and handle it individually?
There was a problem hiding this comment.
@PCloud63514 Please start by adding a comment block to document the function.
There was a problem hiding this comment.
Added a doc comment for requireCommentParseIf to clarify its purpose.requireCommentParseIf
| for _, line := range lines { | ||
| trim := strings.TrimSpace(line) | ||
|
|
||
| if !inIf { | ||
| slash := strings.Index(line, "//") | ||
| if slash < 0 { | ||
| out = append(out, line) | ||
| continue | ||
| } | ||
|
|
||
| trimmed := strings.TrimSpace(line[slash+2:]) | ||
|
|
||
| if strings.HasPrefix(trimmed, "if require.") && strings.HasSuffix(trimmed, "{") { | ||
| commentPrefix = line[:slash] + "//\t " | ||
|
|
||
| h := strings.TrimPrefix(trimmed, "if ") | ||
| h = strings.TrimSpace(h) | ||
| if strings.HasSuffix(h, "{") { | ||
| h = strings.TrimSuffix(h, "{") | ||
| } | ||
| h = strings.TrimSpace(h) | ||
|
|
||
| out = append(out, commentPrefix+strings.TrimLeft(h, "\t")) | ||
| inIf = true | ||
| continue | ||
| } | ||
| out = append(out, line) | ||
| } else { | ||
| if strings.HasPrefix(trim, "//") { | ||
| body := strings.TrimSpace(strings.TrimPrefix(trim, "//")) | ||
| if body == "}" { | ||
| inIf = false | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| slash := strings.Index(line, "//") | ||
| if slash >= 0 { | ||
| body := strings.TrimLeft(line[slash+2:], " \t") | ||
| out = append(out, commentPrefix+body) | ||
| } else { | ||
| out = append(out, line) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return strings.Join(out, "\n") | ||
| } |
There was a problem hiding this comment.
I'm worried about a badly indented comment.
I'm unsure how to handle this correctly.
|
@ccoVeille Thanks, I also felt the code was a bit messy, so I tried to simplify it further. Regarding your comment about tests: since this is only used for code generation, |
_codegen/main.go
Outdated
| commentPrefix := rePrefix.FindString(line) | ||
| comment := strings.TrimSpace(line[2:]) | ||
|
|
||
| if strings.HasSuffix(comment, "SomeFunction()") { |
There was a problem hiding this comment.
I don't like the explicit reference to SomeFunction(). This should instead look for the first line of an example block by looking at indent. But maybe there is a reason why this wasn't enough?
There was a problem hiding this comment.
Hi @dolmen
I’ve removed the explicit SomeFunction() reference.
That logic was originally added considering the type S struct { comment,
but now the handling is simplified to focus only on if require. cases.
dolmen
changed the title
dolmen
added
internal/codegen
|
Is there anything more I should do, or anything I might be misunderstanding? |
4 participants
Summary
Correct invalid examples in require package doc comments.
require does not return a boolean value; its responsibility is to fail immediately.
Changes
Updated examples to reflect proper require usage without if.
Added a new helper requireCommentParseIf to sanitize and transform invalid doc comments in bulk, ensuring consistency across the package.
Removed conditional forms such as:
Updated examples to demonstrate proper usage of require:
Motivation
Unlike assert, require functions do not return a boolean value.
The previous examples suggested patterns such as if require.NoError(t, err) { ... }, which are invalid.
This PR fixes those examples and introduces an automated parser (requireCommentParseIf) to prevent such mistakes from persisting in the future.
Related issues
Closes #1776