Skip to content

Conversation

semihbkgr
Copy link
Contributor

Closes #1231

Fixed the exported rule to correctly handle and exclude deprecation-only comments.

Previously, when only a deprecation comment was present, it incorrectly reported:
comment on exported type ... should be of the form;
now it reports:
exported type ... should have comment or be unexported.

// e.g. //go:embed foo.txt a directive comment, not a text comment
// e.g. //nolint:whatever is a directive comment, not a text comment
func hasTextComment(comment *ast.CommentGroup) bool {
// e.g. // Deprecated: this is a deprecation comment
func hasDocComment(comment *ast.CommentGroup) bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expanded the logic of this function to check that the comment contains a non-empty text comment and is not solely a deprecation comment. I renamed it to reflect this change. I can adjust the name if you have a better suggestion.

@chavacava
Copy link
Collaborator

Hi @semihbkgr thanks for the PR!

I've added a test case to the PR. The case is that of a function with a deprecation comment line followed by a wrong comment line (Minimum ... where Min ... is expected). The expected failure message for that case is comment on exported function Min should be of the form "Min ..."

@semihbkgr
Copy link
Contributor Author

Hi @chavacava

I assumed that everything following the Deprecated: tag is part of the deprecation comment. However, I'm unsure whether placing the deprecation comment before the doc comment—as done in the new test case you added—is correct. I couldn’t find any examples of this ordering in popular codebases. Typically, the doc comment appears first, followed by the deprecation comment. I'm not certain whether this is a formal rule or just a widely followed convention.
Also, if the doc comment comes after the deprecation comment, it becomes unclear how to distinguish between the deprecation comment and the doc comment.

@chavacava
Copy link
Collaborator

The official doc on deprecation comments says:

Deprecation notices are followed by some information about the deprecation, and a recommendation on what to use instead, if applicable. The paragraph does not have to be the last paragraph in the doc comment.

// Package rc4 implements the RC4 stream cipher.
//
// Deprecated: RC4 is cryptographically broken and should not be used
// except for compatibility with legacy systems.
//
// This package is frozen and no new functionality will be added.
package rc4

Let's assume that the deprecation "section" is always after the "documentation" section.
Feel free to modify the test case I've added to the PR.

@semihbkgr
Copy link
Contributor Author

semihbkgr commented Apr 6, 2025

So we can assume that the doc comment comes before the deprecation comment - this isn’t a strict rule, but it’s a commonly followed convention.
Alternatively, we can treat only the paragraph starting with the Deprecated: tag as the deprecation comment, and consider the following paragraphs as part of the doc comment.
This would be another assumption (since we don't know whether the deprecation comment consists of multiple paragraphs), but it allows us to handle cases where the doc comment appears after the deprecation comment. For example:

// Deprecated: this is deprecated, use min instead -> deprecation comment
//                                         -> end of paragraph
// Min returns a if a <= b, b otherwise.            -> doc comment
func Min(a, b int) int {

@chavacava
Copy link
Collaborator

Let's keep it simple and stick to the convention (comment + deprecation)

@chavacava chavacava merged commit c882cb8 into mgechev:master Apr 6, 2025
8 checks passed
@ccoVeille
Copy link
Contributor

Thank you guys for working on this. I was out with family, I only review it now.

mfederowicz pushed a commit to mfederowicz/revive that referenced this pull request Apr 18, 2025
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.

exported: comment starting with "Deprecated:” should be ignored when checking if a comment is present
3 participants