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

Apply swift-format to StripeCore #2055

Merged
merged 4 commits into from
Nov 3, 2022
Merged

Apply swift-format to StripeCore #2055

merged 4 commits into from
Nov 3, 2022

Conversation

eurias-stripe
Copy link
Contributor

Summary

Applying swift-format to StripeCore.
First step to enable swift-format as a CI check.
Started with this project since is the lowest in the pyramid and relatively small. Most of the changes you'll see are in doc comments, which I sadly had to make manually since I didn't find any tool that can fix those automatically, and since it was quite tedious to do, I think I will have to disable documentation related checks at least to begin with.
On everything else, I used most of the default configuration with just a couple of settings changed:

  • AlwaysUseLowerCamelCase and OnlyOneTrailingClosureArgument because we have plenty of cases in our codebase that violate those rules, the first one mostly because of the STP prefix, and it wouldn't make sense to try and change all of that just to enable these settings.
  • lineBreakBeforeEachArgument and prioritizeKeepingFunctionOutputTogether are just a personal preference, which is open for discussion if anyone has a different preference.

In the end any settings we decide to use don't matter much, the point is using a common style and let the formatter take care of it so we don't have to.

Motivation

Establish style guidelines.

Testing

  • Build
  • CI

@davidme-stripe
Copy link
Contributor

This looks great! I'm not picky on the actual settings either. Added one comment, it looks like the formatter did something weird with our _spi usage.

I added this while I was formatting locally but it shouldn't be committed yet.
@davidme-stripe
Copy link
Contributor

Can we remove whatever rule is removing :nodoc:? We have a few extensions that need to be marked as public but shouldn't clutter up our documentation. (e.g. UnknownFieldsDecodable)

@eurias-stripe
Copy link
Contributor Author

That was me removing them manually because that doc line is invalid, it should be in the form of a sentence.
I can turn off all the rules related to documentation.
What happens if they aren't maked with :nodoc:? I assumed that if there was no documentation it would simply not be generated, is the :nodoc: annotation required for some tool?

@davidme-stripe
Copy link
Contributor

:nodoc: tells Jazzy not to generate documentation for a function. Without :nodoc:, it'll fail with an error if any public functions are missing documentation. (But that may not be working properly — this PR should have failed the check-docs build!)

Looks like Swift is adding an official feature for this to the next release: https://forums.swift.org/t/nodoc-attribute-for-hiding-symbols-from-the-symbol-graph/59227/49

@euriasb
Copy link

euriasb commented Nov 3, 2022

Ok. I can either add swift-format-ignore to the :nodoc: annotations or disable documentation related rules. I'm okay with either.

@davidme-stripe
Copy link
Contributor

I think we should hold off on the documentation-related rules for now — we have a lot of :nodoc:s. We could also fork swift-format, but that might be more trouble than it's worth.

@eurias-stripe
Copy link
Contributor Author

Yeah, I'm gonna disable them.
Also, I just realized I commented from my personal account lol, welcome to the public repo!

@eurias-stripe eurias-stripe merged commit fe5bfb8 into master Nov 3, 2022
@eurias-stripe eurias-stripe deleted the eurias/swift_format branch November 3, 2022 20:31
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.

3 participants