-
Notifications
You must be signed in to change notification settings - Fork 41
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
Changed to match current library distribution status #213
Changed to match current library distribution status #213
Conversation
Thanks @psolstice, looks good. Few questions, see below so that we can finalise: |
@psolstice actually, I think we need to look at a few other things. Firstly, if you look at the installation instructions, firstly it seems we recommend downloading a JAR over using Graven etc. See below: Also, it says you can download a JAR, yet I thought for Android it's now an AAR file? Can you advise? Finally, if you look at the releases page, it seems we're not following our release instructions firstly (it's out of date), and secondly we have no AAR file (related to previous point). Should we not have an AAR as well with all dependencies? |
@@ -85,9 +85,21 @@ blang[ruby]. | |||
``` | |||
|
|||
blang[java]. | |||
The Realtime library for Java and Android is "hosted on Github":https://github.com/ably/ably-java and is downloadable as a JAR from "https://github.com/ably/ably-java/releases":https://github.com/ably/ably-java/releases or via "JCentre":https://bintray.com/ably-io/ably. | |||
The Realtime library for Java and Android is "hosted on Github":https://github.com/ably/ably-java and can be used by adding one line to <span language="default">@build.gradle@ dependencies section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the erroneous span for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, copy-pasted part of code from another place without understanding the formatting
compile 'io.ably:ably-android:0.8.7' | ||
``` | ||
|
||
Ensure the library is included in your classpath as follows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there not a way to not fix into a specific version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compile "io.ably:ably-android:0.8.+" ? @psolstice ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to do it this way but some IDEs (e.g. Android Studio) will give a warning and strongly discourage you from doing so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psolstice @mattheworiordan I agree it is discouraged so although it's a pain I think on balance it is probably better to update the tutorials and docs when there are new releases; hopefully the releases will settle down so it's not happening too often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paddybyers that sounds like a nightmare, especially as that couples releases of Java with the docs repo. Personally I'd prefer that we at least recommend that users simply check for the latest version with a link to the tags / releases for the Java repo so that they can see the versions. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine. I don't think the docs and tutorials always have to point to the latest, but equally we'll need to ensure they're reasonably up to date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the action here is to add some comments about replacing with the latest version? Also, I assume now unfortunately #215 (comment) is incorrect. If so, that's a huge pain, see https://github.com/ably/tutorials/pulls to get an idea why, that's 6 PRs to fix all the commit SHAs that are then referenced from #215 (comment)
@@ -106,9 +106,21 @@ blang[php]. | |||
``` | |||
|
|||
blang[java]. | |||
The REST library for Java and Android is "hosted on Github":https://github.com/ably/ably-java and is downloadable as a JAR from "https://github.com/ably/ably-java/releases":https://github.com/ably/ably-java/releases or via "JCentre":https://bintray.com/ably-io/ably. | |||
The REST library for Java and Android is "hosted on Github":https://github.com/ably/ably-java and can be used by adding one line to <span language="default">@build.gradle@ dependencies section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, why the span?
@@ -50,9 +50,21 @@ blang[php]. | |||
bc[php]. require_once __DIR__ . '/../vendor/autoload.php'; | |||
|
|||
blang[java]. | |||
The Ably library for Java and Android is downloadable as a JAR from "https://github.com/ably/ably-java/releases":https://github.com/ably/ably-java/releases or via "JCentre":https://bintray.com/ably-io/ably. | |||
The Ably library for Java and Android is "hosted on Github":https://github.com/ably/ably-java and can be used by adding one line to <span language="default">@build.gradle@ dependencies section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
@@ -45,11 +45,10 @@ blang[java]. | |||
|
|||
repositories { | |||
jcenter() | |||
maven { url "https://raw.github.com/paddybyers/Java-WebSocket/mvn-repo/" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need jcenter() then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user is using IDE this line is generated automatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fine, just checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paddybyers @psolstice I believe there are still outstanding items to be resolved here. This PR has been open now for 2 weeks. Can we get this resolved and merged in today please as this is blocking #215 (comment) and also means customers are still not seeing the correct docs.
compile 'io.ably:ably-android:0.8.7' | ||
``` | ||
|
||
Ensure the library is included in your classpath as follows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the action here is to add some comments about replacing with the latest version? Also, I assume now unfortunately #215 (comment) is incorrect. If so, that's a huge pain, see https://github.com/ably/tutorials/pulls to get an idea why, that's 6 PRs to fix all the commit SHAs that are then referenced from #215 (comment)
compile 'io.ably:ably-android:0.8.7' | ||
``` | ||
|
||
Ensure the library is included in your classpath as follows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we not agree we'd mention that the version number should be updated with perhaps links to the latest version numbers?
LGTM. @paddybyers you happy for me to merge? |
Yes |
No description provided.