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

Update documentation style recommendations #9

Merged
merged 2 commits into from
Aug 10, 2020

Conversation

aniakorolova
Copy link

The new auto-generated Xcode docs use /// for both single- and multi-line comments, so our recommendations are a little outdated.

Copy link
Contributor

@rastersize rastersize left a comment

Choose a reason for hiding this comment

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

I think this could be a controversial change. Since we have a lot of code using the previous /** <docs> */ format, and Apple has been flip-flopping between the three styles - ///, /** */ and /** */ (with * for each line).

Is there a plan for how to migrate to /// if this change is accepted?
Can we, somehow, mitigate Apple flip-flopping again?
How can we make sure there’s (enough) alignment in the guild around this?

I personally think following Xcode’s default is a good idea, but there’s the issue of our API docs getting diverging in style.

README.md Outdated
or method and use `Cmd`-`/` to see this in action. Make sure to use the available markup tags like `@param`,
`@return`, etc. (these will be auto-generated for you by Xcode). The `///` form is preferred for single line
comments, and `/** */` for multi-line comments. Use the same formatting as in the example block above.
or method and use `Option`+`Cmd`+`/` to see this in action. Make sure to use the available markup tags like `@param`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to mention the format we want. For those that aren’t using this specific version of Xcode, or Xcode at all. I.e. adding a mention like “Use /// (three forward slashes) for single and multi-line comment.

Copy link
Author

@aniakorolova aniakorolova Jun 16, 2020

Choose a reason for hiding this comment

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

I actually thought it would be easier to just follow the current Xcode format, whatever that is. I agree though, that it could be unclear for someone who's using another IDE. Do we know how many people are?

Copy link
Member

@BalestraPatrick BalestraPatrick Jul 7, 2020

Choose a reason for hiding this comment

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

AFAIK, only one person is not using Xcode every now and then, so we can assume that everyone is using Xcode :P

Choose a reason for hiding this comment

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

I would also recommend /// specifically instead of the hotkey as I expect these to not be changed soon
BTW, the markups aren't @param and such anymore. It's Parameter:, Return: and so on:

    /// Description
    ///
    /// - Parameters:
    ///   - withOperations: The array of content operations to use to display content.
    ///   - type: The desired type of the resulting `SPTHubViewController`.
    ///
    /// - Returns: The resulting `SPTHubViewController` that's initialized as `type`.

I would also maybe recommend a styling approach to it (like a blank line between the sections like above) as I think the generated Xcode one doesn't do so and it looks a bit weird IMO

Copy link

Choose a reason for hiding this comment

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

@rockbruno it is still @param for ObjC.

@aniakorolova
Copy link
Author

aniakorolova commented Jun 16, 2020

There is no plan for migrating the old docs, AFAIK. I raised this question on guild meetings twice and the best suggestion I got was to make a PR with this change. So far, it seems to be a better way to start a discussion :)
It feels like an unnecessary effort to add new docs manually, because the Xcode formatting is different. In my squad we keep doing that, although I know that some other squads just switched to the new format without migrating the old docs.

@rockbruno
Copy link

I think it's nice to recommend ///. AFAIK this is the official Swift format, so I don't see it going away this time.
One specific thing that I was doing however was to still use /** */, @param and such in Objective-C code. Maybe it's better to separate these by language?

@bubski
Copy link

bubski commented Jul 8, 2020

I'm very in favor of this change.

Is there a plan for how to migrate to /// if this change is accepted?

I'm open to give it a shot, would like seeing that happen too, but can't make promises.

However, to me the greatest value of merging this is in writing documentation, not in reading it, so I wouldn't want to see mass-migration as a blocker for this.

Can we, somehow, mitigate Apple flip-flopping again?

I believe Xcode's plugin system should enable us to inject custom documentation templates, but I do not think it's worth the effort, and, to me at least, misses the point of aligning with the Xcode's default.

How can we make sure there’s (enough) alignment in the guild around this?

As @aniakorolova said, she'd already communicated that several times in the guild meetings.
I think we should set a deadline for people to voice their objections, and if there are none, merge it.

@rastersize
Copy link
Contributor

I’m satisfied with the answers @bubski provided for my overall questions. I do still think we should specific the style of comments we’d like to use though. If/when Apple changes their mind (again) on what option+cmd+/ uses we can update the style guide again.

@poncel
Copy link
Contributor

poncel commented Aug 10, 2020

I'm in favour of merging this, since most teams are using what Xcode provides by default. @aniakorolova Could you add a comment to make it explicit what the current Xcode formatting does and that if that changes we should update the guideline?

Copy link
Member

@BalestraPatrick BalestraPatrick left a comment

Choose a reason for hiding this comment

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

Seems like people are pretty much in favour of this, so looks good to me as well.

@aniakorolova
Copy link
Author

aniakorolova commented Aug 10, 2020

I added the mentioning of ///, I think that was the most requested change.
@rockbruno just to clarify, this is the Obj-C style guide. There's also the Swift style guide in the Backstage docs.
I'm going to merge this now!

edit: can't merge it actually, I don't have the write access.

@poncel poncel merged commit de5f686 into spotify:master Aug 10, 2020
@aniakorolova aniakorolova deleted the comment-style branch August 10, 2020 14:44
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.

6 participants