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

Revisions to Android Training pages #408

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

paulsmithkc
Copy link
Contributor

Broke the PR into two commits as requested. First commit is the initial conversion to markdown. Second commit is the revised text of the training pages.

See #395 and #405 for background.

@paulsmithkc paulsmithkc changed the title Docs 2 Revisions to Android Training pages Apr 1, 2021
@jpd236
Copy link
Collaborator

jpd236 commented Apr 1, 2021

Thanks!

This is definitely reviewable, but fairly substantial. It will take some time to go through this all and determine what is/isn't suitable for the general documentation.

@jpd236
Copy link
Collaborator

jpd236 commented Apr 7, 2021

Update: I've been in touch with the team maintaining the developer.android.com docs, and we are planning to move the docs over to GitHub (with a redirect at DAC for anyone who still goes there) in the near future. Our internal source is similar to Markdown, so we'll be importing from there for the initial docs - that way we're using the original source and hopefully keeping things higher fidelity. Once that's done, we're also hoping they'll be able to help review these proposed doc changes, but you'll likely need to do some rebasing of your edits on top of the new Markdown files since there'll likely be some differences between your generated conversions and our new source files. Just wanted to give a heads up as you may want to hold off on further changes to avoid churn when the new sources are available.

@jpd236
Copy link
Collaborator

jpd236 commented Feb 18, 2022

So sorry this ended up taking so long! But the migrated Markdown versions of the doc are now committed here, so you should be able to rebase your changes on top of them so we can review them properly, if you're still interested in contributing at this point.

From taking a closer skim - it looks like a good portion of the minor tweaks to writing / code snippet are probably improvements, though there are a few I'd disagree with. I don't think the advice on avoiding singletons makes sense here - RequestQueues are expensive to spin up/down, and you do not want multiple instances of them in a running application. I'm on the fence about the WorkManager article - on one hand, it's a fairly standard use of WorkManager, which feels out of scope of Volley's documentation, but I can see it being helpful for someone having trouble stitching them together who might fall into the trap of using RequestFuture or otherwise not understanding the async path.

So it might be good to start with just a PR for the minor improvements which should hopefully be less controversial, and we can discuss the WorkManager article as its own piece?

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