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

Fixes #165 - Enable documentation comment validation in swift-format #188

Conversation

PARAIPAN9
Copy link
Contributor

Motivation

This is for better documentation
Fixes #165

Modifications

Enabled some rules in .swift-format

Result

The .swift-format will be changed and the rules will reflect into the project

Test Plan

Run tests and CI

@czechboy0
Copy link
Contributor

Sorry if the issue wasn't clear, part of the enablement will likely need to be to actually update all the places where the docs aren't passing validation. Otherwise our CI would go red.

@PARAIPAN9
Copy link
Contributor Author

There is also this rule: BeginDocumentationCommentWithOneLineSummary that is comment related, currently set to false.

.swift-format Outdated Show resolved Hide resolved
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

This will need updating the actual places missing docs (try running ./scripts/soundness locally). That might be quite a bit of work, so if that seems like too much, feel free to put the issue back into the queue and someone else will pick it up later.

@czechboy0
Copy link
Contributor

There is also this rule: BeginDocumentationCommentWithOneLineSummary that is comment related, currently set to false.

I don't think we need to set that, it's overly strict about the abstract of each symbol. Could be added later.

@czechboy0
Copy link
Contributor

Just setting the PR back to draft to communicate its status.

@czechboy0 czechboy0 marked this pull request as draft August 14, 2023 06:15
@PARAIPAN9
Copy link
Contributor Author

Just a heads up to let you know that I'm still working on it, it will take a little bit of time.

@czechboy0
Copy link
Contributor

No rush, @PARAIPAN9 – thanks 🙏

@PARAIPAN9
Copy link
Contributor Author

Hello, I have another update about this PR. I've come to a stage where I need to include comment documentation for the generated files and by doing this there is reference test that is failing. What would be the best approach here? Should I modify the generator to produce the documentation, similar to how it does it for "Remark" comments?

@czechboy0
Copy link
Contributor

No, let's exclude the files from being reformatted and linted. They get formatted as part of the generation process anyway, so this doesn't remove any value.

I suspect updating the script for formatting should do the trick.

@PARAIPAN9 PARAIPAN9 marked this pull request as ready for review August 23, 2023 14:50
@czechboy0 czechboy0 added this to the Post-1.0 milestone Aug 25, 2023
@czechboy0 czechboy0 marked this pull request as draft August 28, 2023 12:49
@czechboy0 czechboy0 removed this from the Post-1.0 milestone Aug 30, 2023
@czechboy0
Copy link
Contributor

@PARAIPAN9 Once you'd like this reviewed, please move the PR into Ready for review.

@PARAIPAN9
Copy link
Contributor Author

Hello @czechboy0, I will update the branch and after I will also change the status.

@PARAIPAN9 PARAIPAN9 marked this pull request as ready for review October 10, 2023 15:14
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Just one question about the script, otherwise looks great, thank you!

scripts/run-swift-format.sh Outdated Show resolved Hide resolved
scripts/run-swift-format.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Thank you, @PARAIPAN9!

@czechboy0 czechboy0 merged commit 3f8542b into apple:main Oct 11, 2023
@czechboy0
Copy link
Contributor

czechboy0 commented Oct 11, 2023

@PARAIPAN9 If you'd be interested, it'd be good to get this change into swift-openapi-runtime (and the urlsession/ahc transports) as well, but no pressure 🙂

@PARAIPAN9
Copy link
Contributor Author

Sure, no problem!!

@czechboy0 czechboy0 added the semver/none No version bump required. label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable documentation comment validation in swift-format
2 participants