search

google / volley

https://google.github.io/volley
Apache License 2.0
3.38k stars 754 forks source link

Revisions to Android Training pages #408

Open paulsmithkc opened 3 years ago

paulsmithkc commented 3 years ago

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 https://github.com/google/volley/issues/395 and https://github.com/google/volley/pull/405 for background.

jpd236 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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?