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

Docs for NSError extension (specifically) don't get parsed. #502

Closed
freak4pc opened this issue Mar 27, 2016 · 34 comments
Closed

Docs for NSError extension (specifically) don't get parsed. #502

freak4pc opened this issue Mar 27, 2016 · 34 comments
Assignees
Labels

Comments

@freak4pc
Copy link
Collaborator

I have some extensions that are all being parsed correctly, for example, this gets detected with no issues and generates valid HTML docs:

public extension String {
    //MARK: - String Extras

    /// A capitalized address string.
    /// e.g. `5TH AVENUE ST` returns `5th Avenue St`
    public var capitalizedAddress: String {

But specifically my NSError extensions are being skipped for some reason and docs are never generated even though they follow the same format:

public extension NSError {
    //MARK: - NSError Extras

    /// Return a `THResponse` object corresponding to the current `NSError`.
    public var asResponse: THResponse {
    ...
    ...

This is how it looks:
image

This is even weirder since jazzy outputs parsing of the file, but doesn't generate the docs from it e.g. Parsing NSError.Exts.swift (27/47) .

Would appreciate any help on this !

Kind Regards,
Shai.

@freak4pc
Copy link
Collaborator Author

This seems to be the case for UIColor, UIFont, CLLocation extensions as well. But works for Float, String, UIView extensions.

I thought this might be related to Swift types vs. Obj-C types but UIView works fine ...

@segiddins
Copy link
Collaborator

Do you have the necessary import statements in those files?

@radutzan
Copy link

This is happening to me with a UIFont extension. The file has import UIKit at the top. A different file with a UIImage extension doesn't have the issue. I tried moving the UIFont extension to the same file where the UIImage extension is, but no luck.

@freak4pc
Copy link
Collaborator Author

@segiddins what do you mean by that? Those files have the regular imports just like the rest of my files that are recognized. This seems specific to extensions for those classes.

@segiddins
Copy link
Collaborator

In that case, a project that reproduces the issue would be the first step towards debugging.

@freak4pc
Copy link
Collaborator Author

Absolutely. Taking care of that now.

@freak4pc
Copy link
Collaborator Author

I seem to have tracked down the bug.
It doesn't create documentation for that file when I execute with the --skip-undocumented flag.

e.g. this would generate documentation for that file
jazzy --swift-version 2.2 --min-acl internal
image

this won't, even though that file IS documented :
jazzy --swift-version 2.2 --min-acl internal --skip-undocumented
image

Here's my UIColor.Exts.Swift file source

//
//  UIColor.Exts.swift
//  JazzyDocIssue
//
//  Created by Shai Mishali on 29/03/2016.
//  Copyright © 2016 Shai Mishali. All rights reserved.
//

import Foundation
import UIKit

/// This includes extensions for UIColor.
public extension UIColor {
    /// This methods prints the `UIColor` object, uselessly enough.
    public func printColor(){
        print("This is a color ! \(self)")
    }

    /// Returns a cool color.
    public static var coolColor: UIColor { return UIColor.blueColor() }
}

@freak4pc
Copy link
Collaborator Author

This is probably due to the fact it doesn't recognize it being documented. Even without that flag, it writes Undocumented instead of the correct description.

This (bad):
image

Vs this for String (good):
image

@segiddins
Copy link
Collaborator

Ah so jazzy should be documenting children of undocumented extensions always, this looks like a bug!

@freak4pc
Copy link
Collaborator Author

The problem is it actually IS documented... It recognises it as undocumented for some reason , despite recognising String as documented, both follow the same "rules" of documentation...

@radutzan
Copy link

Can confirm that it does produce docs for my UIFont methods when skip-undocumented is off, but does not recognize the documentation for the extension itself (says "Undocumented").

On the other hand, my UIImage extension does not have extension-level documentation, but jazzy still generates a site for the one method inside, even with skip-undocumented (the method is documented but the extension isn't, and the header doesn't say "Undocumented").

@jpsim
Copy link
Collaborator

jpsim commented Mar 29, 2016

Can all of you please try using jazzy built from source from the master branch? We've made changes in this area lately that haven't been released so it'd be great to know if this is already resolved.

@freak4pc
Copy link
Collaborator Author

Yes it works for some things and doesn't for other things.
UIFont, NSError don't but for example UIImage does.
It's weird because both inherit from NSObject, nothing special. Very weird indeed.

@jpsim jpsim added the bug label Mar 29, 2016
@freak4pc
Copy link
Collaborator Author

@jpsim Will try. I'm not exactly sure how but will give it a shot ;)

@jpsim
Copy link
Collaborator

jpsim commented Mar 29, 2016

Thanks! You can follow the instructions at https://github.com/realm/jazzy#development.

@radutzan
Copy link

I compiled master and ran the binary, the issue persists. As proof that I'm on the latest: "module-name" was renamed "module".

@jpsim
Copy link
Collaborator

jpsim commented Mar 29, 2016

Ok, let's keep this open until someone has the time to debug & fix then. Thanks for checking against master, @radutzan!

@radutzan
Copy link

No problem, keep up the awesome work!

@LeoNatan
Copy link

LeoNatan commented Apr 3, 2016

Have this issue as well for my category. A solution is not always straight forward, because if there are multiple categories/extensions on the same class, they are merged together. In that case, which documentation should be used?

@LeoNatan
Copy link

LeoNatan commented Apr 3, 2016

One quick fix is to disable the following lines in sourcekitten.rb:

def self.make_doc_info(doc, declaration)
      return unless should_document?(doc)
      # unless doc['key.doc.full_as_xml']
      #   return process_undocumented_token(doc, declaration)
      # end

      declaration.line = doc['key.doc.line']
      declaration.column = doc['key.doc.column']
      declaration.declaration = Highlighter.highlight(
        doc['key.doc.declaration'] || doc['key.parsed_declaration'],
        Config.instance.objc_mode ? 'objc' : 'swift',
      )
      declaration.abstract = Jazzy.markdown.render(doc['key.doc.comment'] || '')
      declaration.discussion = ''
      declaration.return = make_paragraphs(doc, 'key.doc.result_discussion')

      declaration.parameters = parameters(doc)

      @documented_count += 1
    end

This creates the documentation correctly.

Not sure what key.doc.full_as_xml is, or what other side effects this has.

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 3, 2016

I'm not sure that's a quick fix, since that would also break actual undocumented documents, If I'm not mistaken.

@LeoNatan
Copy link

LeoNatan commented Apr 3, 2016

Perhaps determining if a document is documented should be determined differently?

@LeoNatan
Copy link

LeoNatan commented Apr 3, 2016

@freak4pc What if it is changed like so:

    def self.make_doc_info(doc, declaration)
      return unless should_document?(doc)
      if doc['key.doc.comment'].nil?
        return process_undocumented_token(doc, declaration)
      end

      declaration.line = doc['key.doc.line']
      declaration.column = doc['key.doc.column']
      declaration.declaration = Highlighter.highlight(
        doc['key.doc.declaration'] || doc['key.parsed_declaration'],
        Config.instance.objc_mode ? 'objc' : 'swift',
      )
      declaration.abstract = Jazzy.markdown.render(doc['key.doc.comment'] || '')
      declaration.discussion = ''
      declaration.return = make_paragraphs(doc, 'key.doc.result_discussion')

      declaration.parameters = parameters(doc)

      @documented_count += 1
    end

Quickly tested, and it seems to correctly display "Undocumented" if no documentation is provided, but also correctly documents extended classes.

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 3, 2016

@jpsim @segiddins What do you guys think? Could this be submitted as a PR ?

@segiddins
Copy link
Collaborator

If it doesn't break any other tests, sure.

@LeoNatan
Copy link

LeoNatan commented Apr 3, 2016

I'm not sure how to test. My usage is very limited.

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 3, 2016

@LeoNatan , let me take care of that, write up a PR for this and I'll submit it and mention you, if that's ok ?

@LeoNatan
Copy link

LeoNatan commented Apr 3, 2016

Sure, no problem.

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 3, 2016

@LeoNatan Tried your solution but it seems that makes it worse... Now it only documents my UIView extension and skips even the single ones it did before. This doesn't seem to be the right solution ... need to keep digging :)

@LeoNatan
Copy link

LeoNatan commented Apr 3, 2016

Hehe, as I said, my use cases are very limited and it seems to have worked for me. Can you post an example project where this reproduces? I may try to look further tomorrow.

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 3, 2016

Tracked it down, submitting a PR in a bit. ;)
Ruby isn't my main language but I think it's a good solution and it works for my very-heavy project.

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 3, 2016

Uhm that seems to break some of the tests . I'm not sure exactly why. If one of you guys perhaps can assist in making this compatible?

@jpsim
Copy link
Collaborator

jpsim commented Apr 4, 2016

Uhm that seems to break some of the tests . I'm not sure exactly why. If one of you guys perhaps can assist in making this compatible?

Let's continue the conversation in #508. I'm not in my "jazzy head space" right now, so I haven't fully understood the ramifications of such changes, but I'll be able to review further once I can see the changes as discussed in #508.

jpsim pushed a commit that referenced this issue Apr 6, 2016
@pigeon-archive pigeon-archive self-assigned this Nov 23, 2016
@pigeon-archive
Copy link
Contributor

Let's continue the conversation in #508.

Closing since the conversation has continued in a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants