Pull requests

#1 Declined
Repository
alaska alaska
Branch
master
Repository
liamstask liamstask
Branch
master

Add Main()

Author
  1. alaska
Reviewers
Description

(s *GalleryService) Main(section, sort, window string, page int, showViral bool)

Allows you to access the main gallery by section and to sort it and select whether you want viral images shown or not. Useful for streaming or scraping the main gallery.

Comments (5)

  1. Liam Staskawicz repo owner

    Thanks - looks good! Couple questions:

    • should this routine be called Gallery() or Images() instead of Main()? It looks like the main gallery is used as an example of just one of the galleries that this endpoint can return, based on https://api.imgur.com/endpoints/gallery. Its more general purpose is to fetch the images in a gallery, and keeping the names aligned with that doc would be ideal.
    • can you add a test, similar to the others in gallery_test.go?
    • can you gofmt the code?
    • i'm not sure i agree with the change that moves the declaration of defaultBaseURL - would you mind dropping that commit from the PR?
  2. alaska author
    • I was kind of curious about that myself. If you include the '/r' (for subreddits) or the '/g' (for memes) as part of the section specifier, then their API spec starts to look a lot less confusing and a lot more generic. This is the main difference between this route and the rest of them. Making it more generic, then, you could do away with the Random() routine as well, and "random/random" would then become the section. Or, you could potentially unify everything to call the Gallery() method with hard coded '/random' and '/g' prefixes. I agree, it should probably be called Gallery()
    • Absolutely.
    • Shamefully, I admit that the ONE time I didn't run gofmt -w on it was the commit right before I pushed. Gotta fix that .vimrc
    • The main reason I did this is that userAgent is derived from libraryVersion, both of which are arguably more "fundamental" to the library than the base URL (if only slightly), but I'd be happy to remove it; it's your library.
  3. Liam Staskawicz repo owner

    Yeah - I agree the organization of their API leaves a little bit to be desired, but my general approach was to more closely mirror it for transparency's sake, rather than create another layer to understand. Maybe Gallery() is the way to go for now.

    I could go either way on the defaultBaseURL - I was just reacting to what felt like churn. Probably fine to leave it as you have it.

  4. alaska author

    I'm going to decline this (I guess I can do that), and re-abstract the code today and issue another pull request. Is that how things are done around these parts?

  5. Liam Staskawicz repo owner

    That works. I think you can also either a) continue pushing commits to your branch and they'll be part of the PR or b) re-checkout a fresh brach locally then force push to the existing branch, such that the PR then consists only of the commits in the forced push. Whatever works for you is fine by me.