Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AllPublicDeclarationsHaveDocumentation warns on overridden methods #651

Closed
aperkov opened this issue Oct 16, 2023 · 1 comment · Fixed by #655
Closed

AllPublicDeclarationsHaveDocumentation warns on overridden methods #651

aperkov opened this issue Oct 16, 2023 · 1 comment · Fixed by #655
Assignees

Comments

@aperkov
Copy link

aperkov commented Oct 16, 2023

I recently used Homebrew to upgrade to swift-format 509.0.0.

The behaviour of the AllPublicDeclarationsHaveDocumentation rule has changed:

  • Previously overridden methods did not cause the rule to trigger a warning.
  • Now they do.

I'm not sure if this was intentional or not - there doesn't seem to be any mention in the release notes.

The behavioural change seems to have happened in #609

AllPublicDeclarationsHaveDocumentation.swift now contains:

guard
       DocumentationCommentText(extractedFrom: decl.leadingTrivia) == nil,
       modifiers.contains(anyOf: [.public, .override])

Whereas previously it contained a guard that modifiers does not contain override:

guard
       DocumentationCommentText(extractedFrom: decl.leadingTrivia) == nil,
       let mods = modifiers, mods.has(modifier: "public") && !mods.has(modifier: "override")
@allevato
Copy link
Member

Thanks for the report! I noticed that we don't actually have test coverage for this case, and in the process of adding test coverage for it, it revealed that the rule—and a few others—were written incorrect to support unit testing anyway. So this became a whole... thing.

I'll do a targeted fix for this specific issue and then fix the other underlying issues with the tests separately, since it might have broader-ranging impact, and I can cherry-pick the specific fix into a 509 dot release.

@allevato allevato self-assigned this Oct 19, 2023
allevato added a commit to allevato/swift-format that referenced this issue Oct 19, 2023
…methods.

This was mistakenly broken during a refactor and we didn't have test coverage
for it. We didn't have test coverage for it because the tests behave
differently than the rule does when executed in the whole pipeline, so we
need to fix that too. I'm going to do that in a separate PR, because it
might have knock-on effects elsewhere.

Fixes swiftlang#651.
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 a pull request may close this issue.

2 participants