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

Tracking for Treatment of Undocumented Symbols #33

Closed
wants to merge 10 commits into from

Conversation

SDGGiesbrecht
Copy link
Contributor

@SDGGiesbrecht SDGGiesbrecht commented May 26, 2017

This is the result of discussions in realm/jazzy#819 and realm/jazzy#825.

This proposal creates a sort of manifesto for the desired treatment of undocumented symbols, and in particular the nuances of --skip-undocumented and undocumented.json.

It adds a new project to the test suite with symbols in various situations, each named according to the treatment expected, and accompanied by a description of the rationale. It covers many nesting combinations of types, protocols, extensions and methods, each documented, undocumented or with :nodoc:. An emphasis has been placed on corner cases and exceptions.

Because of the way Jazzy’s tests are set up, whenever any pull request affects this treatment, it will trigger diffs which will bring it to the developer’s attention. Because the intentions are present right there in the triggered diffs, the developer will be able to see either that a currently unsupported edge case was fixed (hurray!), or that a regression occurred (oops!). Because the rationale is also right there in the diffs, the new contributor does not need to ask about it, the veteran contributor does not risk forgetting it, and neither has to hunt through old issues and pull requests to find out why it mattered.

This pull request does not attempt to actually add support for all corner cases immediately.

Neither does this pull request imply that minds cannot be changed or decisions retracted in the future. It merely ensures that the existing reasoning is harder to inadvertently overlook or forget.


These expectations mostly derive from various current and past issues. A few instances, such as the combination of :nodoc: with extension have never been mentioned, so I extrapolated what seemed most consistent with the other expectations.

@johnfairh, @jpsim, I would particularly like you two to look over it while the discussion is fresh on your minds in case I forgot something or if there is something you want to change.


I will add a corresponding pull request to Jazzy that assimilates these new test fixtures.

@johnfairh
Copy link
Collaborator

Without getting into the policy claims made by the tests, here are some notes. The files-changed view is a bit overwhelming due to the website parts so putting them here....

At the level jazzy works, there is no difference between struct/class/enum/protocol. I don't think we need repeat all the tests for each: it makes the problem seem more complex than it is and increases the cost of maintaining them.

Similarly, the effect of :nodoc: is to skip the declaration + all children, independent of the type of the declaration. So I don't think there is benefit to having both OptedOutExtensionToLocalOptedOutStructure.swift and OptedOutExtensionToLocalDocumentedStructure.swift, for example.

Similarly+, I'm not sure the repeating pair of should_be_HIDDEN_and_NOT_WARN_ methods is useful more than once: it's the same test every time and and is maybe distracting from the point the file is making.

The Dependency module is probably not being seen as external and so not behaving like UIKit etc. due to an open issue I can't find right now to do with pathnames. Use UIView instead?

The --min-acl is set by default to public. This means that the private + internal declarations are dropped and so the extensions that are not supposed to be empty are in effect empty. Actually I'm not sure what is supposed to be happening in EmptyUndocumentedExtension.swift.

Missing the case that started all this: documented extension of an external undocumented type?

The UndocumentedExtension_Should_NOT_WARN<Component> class has a :nodoc: in its doc comment, which I think is unintended? Other places too, eg. the method in UndocumentedExtensionToLocalUndocumentedStructure.swift that is supposed to be documented.

I appreciate the 'u's but I think US spelling is preferred 😉

Some .DS_Store's have been accidentally committed.

I do think having these behaviors/tests all in one place is useful (although from my limited experience the collection of real-world projects does exercise enough cases) -- personally I would prefer to see these distilled down to one or two swift files in the misc_jazzy_features project: although that project does not have --skip-undocumented, that flag means "instead of saying 'Undocumented', skip this + children" & Alamofire has it turned on so changes to it show up there.

@SDGGiesbrecht
Copy link
Contributor Author

Without getting into the policy claims made by the tests

Mostly I’m just proposing a way to track whatever those policy claims end up being. To demonstrate how the method works, I needed to claim something.

I am hoping such policy decision eventually get concretely defined by someone, because I can easily imagine people submitting conflicting issues, or fixing one issue overzealously and inadvertently regressing on other corner cases.

The files-changed view is a bit overwhelming due to the website parts so putting them here....

That’s why I wanted to do a real demonstration, but because of the following note you made, it didn’t work.

The Dependency module is probably not being seen as external and so not behaving like UIKit etc. due to an open issue I can't find right now to do with pathnames. Use UIView instead?

Thanks especially for that note. I was confused as to why your changes in realm/jazzy#825 did not actually register here as changing anything, but I guess that would be why. If the two situations behave differently now (intended or not) maybe both should be tracked?

Use UIView instead?

That is what I did at first, but then I realized it was not self‐documenting when it appears (correctly or incorrectly) in undocumented.json, so I wanted a more descriptive symbol name.

Actually I'm not sure what is supposed to be happening in EmptyUndocumentedExtension.swift

EmptyUndocumentedExtension.swift in Dependency was supposed to provide an external undocumented symbol for EmptyUndocumentedExtension.swift in the main module. There it has three undocumented extensions, one with only a private method, one with only an internal method, and one with a public but :nodoc: method. The idea is to track whether those situations are correctly being interpreted as “empty”, and consequently being ignored. However, because of the issue you mentioned, I doubt they actually do what they are supposed to yet.

Missing the case that started all this: documented extension of an external undocumented type?

Theoretically, that is in DocumentedExtensionToExternalStructure.swift. I did not differentiate whether or not external types are documented, since I don’t think Jazzy goes fishing for external documentation. Does it?

no difference between struct/class/enum/protocol

Yes, classes and enumerations can be removed in favour of structures. I can’t remember if there are any default implementation examples here, but that is one way protocols are already treated differently.

So I don't think there is benefit to having both OptedOutExtensionToLocalOptedOutStructure.swift and OptedOutExtensionToLocalDocumentedStructure.swift

There were so many combinations that to reduce my own confusion, I simply supplied the entire matrix, so I could tell whether or not I had considered everything. Redundancies can be removed where there are no related exceptions.

Some .DS_Store's have been accidentally committed.

I saw that, but there is no .gitignore in this repository, so I was not sure what was expected, or even if the CI would reproduce them and complain that they did not match. Should a .gitignore be added, or just the .DS_Store files removed?

one or two swift files in the misc_jazzy_features project: although that project does not have --skip-undocumented

--skip-undocumented is necessary for this tracking to work, but it cannot be used with the misc_jazzy_features

I will make some adjustments and see if I can get a demonstration working now that I know about the issue you mentioned that caused it not to work the first time.

@SDGGiesbrecht
Copy link
Contributor Author

The UndocumentedExtension_Should_NOT_WARN<Component> class has a :nodoc: in its doc comment, which I think is unintended?

I only just understood what you meant. You are right. Good catch.

@SDGGiesbrecht
Copy link
Contributor Author

@johnfairh, thanks to your tips, I got it working.

This is a demonstration of the diffs this generates for realm/jazzy#825.

undocumented.json would probably be more legible if it were pretty‐printed instead of just one long line.

@SDGGiesbrecht
Copy link
Contributor Author

SDGGiesbrecht commented May 31, 2017

Doing ⌘F “VISIBLE” on the above mentioned diffs quickly shows that it never occurs in the red, only in the green, meaning nothing is being lost and several fixes are being made.

Doing ⌘F “HIDDEN” shows that it never occurs in the green, meaning nothing unwanted is suddenly showing.

(Note: You have to consciously ignore undocumented.json for now, because, as one long line, the entire thing is in the diffs.)

@SDGGiesbrecht
Copy link
Contributor Author

This was not as quick and easy as I originally thought. It is becoming more work than I really want to invest in something that is just a test. I will close the pull request.

I will leave it up to the project administrators to decide whether to delete the related branches (undocumented-tracking, demonstration, & jazzy/new-test-fixtures) or leave them for someone else to finalize.

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.

2 participants