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

Support documenting "substructures" such as Extensions. #508

Closed
wants to merge 12 commits into from
Closed

Support documenting "substructures" such as Extensions. #508

wants to merge 12 commits into from

Conversation

freak4pc
Copy link
Collaborator

@freak4pc freak4pc commented Apr 3, 2016

This should resolve issue #502 where some documents didn't get recognised as being documented.

This is due to the fact some more complex parts of code/documentation don't have a key.doc.full_as_xml property so they're being discarded as "undocumented", even though some of these have a key.substructure property which provides additional documentation information that should be considered.

@jpsim
Copy link
Collaborator

jpsim commented Apr 4, 2016

Thanks for the PR @freak4pc!

Could you please rebuild the integration specs and make a PR towards realm/jazzy-integration-specs against the jp-sourcekitten-0.11.0 branch which is the most up-to-date branch. That'll make reviewing this easier as we'll be able to see exactly how this affects the build output.

Thanks again!

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 4, 2016

Just want to make sure since I'm not completely up to my Ruby game and dont really know how you guys work :)

  1. Rebuild the integration_specs subrepo
  2. Send a pull request for the specific branch you discussed?

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 4, 2016

Done, I think :) Let me know if that suffices @jpsim

@jpsim
Copy link
Collaborator

jpsim commented Apr 4, 2016

Something went wrong. Try the following:

$ git submodule update --init --recursive
$ [sudo] gem install bundler
$ bundle install
$ bundle exec rake rebuild_integration_fixtures

@jpsim
Copy link
Collaborator

jpsim commented Apr 5, 2016

@freak4pc ok I just fixed the integration specs, upgrading all of them to their respective releases that use Swift 2.2. The integration specs on realm/jazzy-integration-specs should be up to date on master. Could you please rebase this branch, rebuild the integration specs and make a new PR towards realm/jazzy-integration-specs targeting the master branch? I know this is a bit of a convoluted process, so I apologize for that, and thank you for your continued work!

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 6, 2016

@jpsim It's all good, absolutely no worries! I love this project so It's my honour contributing what I can to it.

I just pulled upstream, rebased, rebuilt the integration specs and will now try to send a PR of the integration specs again. I hope it's ok this time, if not - we'll move from there.

Thanks for your assistance !

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 6, 2016

@jpsim Alright, re built the integration specs and pushed a PR :) let me know if there's anything needed on my side. Thanks !

@jpsim
Copy link
Collaborator

jpsim commented Apr 6, 2016

For starters, you'll need to update this repo to point to the updated branch, that way CI will pass (hopefully).

@jpsim
Copy link
Collaborator

jpsim commented Apr 6, 2016

Based on the changes in realm/jazzy-integration-specs#14, this seems right to me! So please update this repo to point to a4df6bc and this should be good assuming CI passes!

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 6, 2016

@jpsim Uhm I think I messed something up :)
I just pulled master, rebased and added the specs commit on top. Am I missing anything?

@jpsim
Copy link
Collaborator

jpsim commented Apr 6, 2016

You're right that your rebase didn't quite work.

I did it myself here (master...jp-freak4pc-substructure-extensions) and refactored the changes into a separate method, appeasing rubocop and rebuilding the integration specs along the way.

You can see the integration specs changes here: realm/jazzy-integration-specs@master...jp-freak4pc-substructure-extensions

That diff highlights some issues with the implementation, specifically that the undocumented parent declarations are no longer being marked as undocumented. This will need to be fixed.

Feel free to continue your work on top of my jp-freak4pc-substructure-extensions branch.

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 7, 2016

That one seems to be a rather quick fix but I also just noticed it creates some undocumented methods, when there are other documented ones in that file. I'm not sure if that's the expected behaviour... seems wrong but I'm not sure where the per-method-testing is being performed.

image

@jpsim
Copy link
Collaborator

jpsim commented Apr 7, 2016

I don't know what's happening there with guard_check() being undocumented and having an entirely wrong declaration. Does that happen just with your branch?

@freak4pc
Copy link
Collaborator Author

freak4pc commented Apr 7, 2016

So, in the master version UIView extensions don't get documented at all.

It seems the code that would consider if a specific method is documented or not might be mis-firing. I'm not sure exactly where it but this seems unrelated to the substructure fix, except for the fact it revealed it.

Interesting though ...

image

@jpsim
Copy link
Collaborator

jpsim commented Apr 14, 2016

Ok. How do you think we should handle that @freak4pc?

@freak4pc
Copy link
Collaborator Author

I need to learn a bit more about the code structure since I already did this "in-point" fix I debugged for the substructures. It's kind of holiday season here so would be a bit tricky making time for this but I plan on tackling it in the following days hopefully :)

@freak4pc
Copy link
Collaborator Author

Haven't had any spare time to deal with this one (Wedding plans, ugh!) .
If anyone wants to take it off my hands, be my guest.
Otherwise I can give this another go in about 3 weeks.

@pigeon-archive
Copy link
Contributor

Hey @freak4pc! Thanks for the response. No worries. If it's your wedding, congrats! _If it's not, that'll be awkward haha 💃 _. We can wait. Looking forward to seeing progress on this in a few weeks. Cheers!

@freak4pc
Copy link
Collaborator Author

Indeed it is. Thanks @istx25 .

@johnfairh
Copy link
Collaborator

Closing because this was finished off in #825.

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

Successfully merging this pull request may close these issues.

4 participants