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

Add dokka artifacts #122 #126

Merged
merged 5 commits into from
Nov 9, 2017
Merged

Add dokka artifacts #122 #126

merged 5 commits into from
Nov 9, 2017

Conversation

charlesdurham
Copy link
Contributor

So there's a lot to validate here but I think I've covered my bases, hopefully lol.

A few things I noticed:

  • Cross module linking isn't implemented with my dokka tasks, but I don't think it's working with javadoc either. This spits out a fair amount of warnings running the documentation tasks.
  • I was able to externally link all the kdoc to rxjava doc, I looked around briefly to try and do the same for the javadoc but didn't find anything that worked; I tried options.link().
  • The source jar for android-kotlin modules was empty in master, I fixed this in 1569705 but don't really know why it wasn't working before.

I split out the artifact tasks into their own files because there's 4 different documentation tasks now;

  • Android + Kotlin
  • Android + Java
  • Kotlin
  • Java

I like this more than adding conditionals to gradle-mvn-push.gradle but I'm not super strongly opinionated so if you feel like it should all go into that file let me know and I'll update.

@CLAassistant
Copy link

CLAassistant commented Nov 5, 2017

CLA assistant check
All committers have signed the CLA.

@charlesdurham
Copy link
Contributor Author

#122

@charlesdurham charlesdurham changed the title Add dokka documentation #122 Add kotlin documentation #122 Nov 5, 2017
@charlesdurham charlesdurham changed the title Add kotlin documentation #122 Add dokka artifacts #122 Nov 5, 2017
Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

This looks pretty neat! I've always preferred using the universal gradle-mvn-push.gradle file, but can also see the value in isolating components. Let me think on this for a couple days (at Droidcon SF today and tomorrow).

CC @kageiit in case you have an opinion

@@ -41,7 +42,9 @@ def build = [
android: 'com.android.tools.build:gradle:2.3.0',
apt: 'net.ltgt.gradle:gradle-apt-plugin:0.11',
errorProne: 'net.ltgt.gradle:gradle-errorprone-plugin:0.0.11',
kotlin: "org.jetbrains.kotlin:kotlin-gradle-plugin:${versions.kotlin}"
kotlin: "org.jetbrains.kotlin:kotlin-gradle-plugin:${versions.kotlin}",
dokka: "org.jetbrains.dokka:dokka-gradle-plugin:${versions.dokka}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: alpha this

@ZacSweers
Copy link
Collaborator

Thinking on it more, I think I'd prefer to keep everything unified in one gradle-mvn-push.gradle file. Is that good with you?

Happy to shout from the rooftops that your contribution is a great example of integration with Dokka in gradle too :)

@charlesdurham
Copy link
Contributor Author

Sure 👍

I consolidated everything into gradle-mvn-push.gradle by creating some functions that add the correct tasks. Not honestly sure if this is a good practice but I felt like it made things easier to read.

Let me know if you want everything inline instead.

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

One comment about the url, otherwise this looks really good!


dokka {
externalDocumentationLink {
url = new URL("http://reactivex.io/RxJava/2.x/javadoc/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, what are you thinking here? That's what builds in linking between the kdoc and rxjava's doc, so if you're looking at documentation that links to rxjava you can click on it and jump straight to the rxjava documentation.

I could see adding https://uber.github.io/AutoDispose/0.x/ being useful (at least until there's proper inter-module linking). Although I think each module's doc might need to be linked independently. The dokka examples indicate you can have multiple external documentation links so if you want I could add it and keep rxjava.

I actually didn't even realize the documentation was hosted at https://uber.github.io/AutoDispose/0.x/, this PR just creates javadoc artifacts for maven.

I see https://github.com/uber/AutoDispose/tree/gh-pages/0.x but don't really know how/if the process of getting the doc in there is automated. If you can point me in the right direction I'd be happy to include that as part of this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I'd held off on publicizing the docs site till dokka was figured out. I upload them via osstrich, unfortunately not sure automating it is viable vs just doing it manually after releases (don't worry about it for this PR). I didn't realize that was for dependencies of the project though, so this makes sense 👍


dokka {
externalDocumentationLink {
url = new URL("http://reactivex.io/RxJava/2.x/javadoc/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

@ZacSweers ZacSweers merged commit f697749 into uber:master Nov 9, 2017
@ZacSweers
Copy link
Collaborator

Thanks! I'll give this a spin at the next release, which should be soon-ish

This was referenced Nov 10, 2017
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