arlo-dl -- feedback about arlo-go

[lrstanley]

Hello! I recently wrote up a quick utility which streamlines the mirroring of recordings from Arlo, using this library: https://github.com/lrstanley/arlo-dl.

Per the readme, I wanted to share some feedback. Loving that I can interact with Arlo's API's in Go now, but I did notice some odd issues/quirks.

1. Go doesn't use openssl (readme still mentions python openssl 😄)
2. Please add the godoc.org badges to the readme.
3. I'd recommend removing the "delete all recordings" section from the readme example. I forgot to comment this out when testing, and I 💀 all of my recordings. Just showing downloading recordings is a good enough example to show in the readme. In addition, I'd recommend placing the example in a folder inside the _examples directory. This is the common location for most packages (example layout here).
4. I notice that you're caching device information on login. I wouldn't recommend doing this, as this isn't standard practice for an API library to do.
   • As an example, you don't see MySQL libraries caching the list of databases it has access to on connect, or more relatable to this, you don't see the MySQL library actually fetch the database rows on connect, and store them in some global state.
   • When the user fetches the devices, the user should know that the function call they just made is live data, not potentially stale data.
   • When initializing, this causes a delay, which a user may not need if they are not actually interacting with the devices themselves.
   • This also creates an issue of having to share/pass around these fields in the Arlo struct.
5. Some of the methods are a bit odd -- for example, Devices.GetCameras(), I would think would return a slice of cameras, not a custom-typed version of a slice of cameras. It's not clear unless the user reads your source, to know they can iterate over that returned type. returning []Camera is more much clear in my opinion.
   • This does restrict the methods you have on this type, but I don't think those are as useful as one might think. In my opinion, it might be better to do arlo.FindDevice(id string) (Device, error) and having it call arlo.GetDevices() under the covers, than calling arlo.GetDevices(), then devices.FindDevice(). This way, the user wanting more control, can just iterate through the devices themselves, and a user wanting simplicity, gets it at the abstraction level.
   • I think this is the case of any type which defines it's own []<type> on top of another type, in most situations. Devices, Cameras, Basestations, etc.
6. In general, passing the client around in every struct (as arlo) is a frowned upon thing in go from my experience, as it reduces simplicity, and can lead to very messy code.
7. The FromUnixMicro style functions defined at package root, aren't things that should, in my opinion, live in this library. If these are needed, they might be better off as methods on the given type that needs them. I.e. Timestamp() method on the Recording type (and if there is a conflicting field, name that TimestampUnix, etc), rather than requiring the user to pass some field into these functions.
8. Unless a method on Arlo is provided to arbitrarily call a given API endpoint, the *Uri constants that are exported here, I don't see why they would need to be exported, given the end user isn't going to use them. This would reduce the type footprint for users.
9. Have you looked into moving to go modules yet?

I am willing to help with some of these points if you think they are valid.

[jeffreydwalter]

Hi @lrstanley, let me say up front, thank you very much for all the great feedback and the offer to help!

My apologies for the delay in getting back to you. I've had more priorities than I have time lately. :)

I will attempt to address all of your comments/suggestions in order:

1. & 2. & 3. All good suggestions. Much appreciated. Haven't had anyone mention accidentally deleting their library running the sample code before, but clearly it's not a great thing to have the potential for. I'm happy to accept a PR for them.

2. The device caching is necessary (imho) in this library because of the nature of the Arlo API. It's fundamentally an async API which uses a single EventStream for all the messages. The EventStream and all related HTTP requests to the /notify endpoint share a single HTTP session, and only a single HTTP session is allowed. I also didn't want to have to pass that session around all over the place. The basestations are the only ones that interact with the EventStream and the basestation must ping the EventStream every 30 seconds to keep it alive. (The ping request includes the basestation device id as well as the xCloudId, which is only found on devices.) So, to me, it made sense to tie the EventStream to the basestations and cache all the devices, but I'm open to having architectural discussions on this topic. If you can propose a better architecture, I'm happy to consider it.

3. The thing I was trying to avoid was forcing the programmer to cast the device to the specific type once they call FindDevice() as that sort of requires the programmer to have explicit knowledge of the underlying, not documented at all, Arlo API and what device types there are. I want to remove as much of that as possible in this library. If you want to throw together a PR with your suggested changes here, I'm happy to consider/discussion it.

4. Again, I'm open to a proposal (or PR to talk about) here. I made this decision because the HTTP session is required for all of the API calls which are made by all of the devices (and the EventStream). It was an architectural decision, as I didn't want to have to injection the HTTP session/request object into every API method, and I wanted the individual API methods to be tied to their respective devices, so it was clear to the consumer of this library which APIs they could call on which objects.

5. I agree. Those functions are just there because it was late and I haven't gotten around to moving them into their own package. At the time, I was fighting with the go core team about getting those functions moved into the go standard library, and so, I just threw them in to get by. But I do like the suggestion of adding methods to those types. I will move them to an internal package soon.

6. Just haven't gotten around to it, but I definitely agree and will put it on the backlog. (I'm happy to accept a PR for this too.)

7. I have, just didn't feel like it was ready at the time, and dep has been working fine, so figured I'd wait until Go 2 or whatever and deal with that then. (This is also something I'm open to consider a PR for, but would want to discuss it a little bit first.)

Thanks again! Jeff

[jeffreydwalter]

@lrstanley going to close this issue. I went ahead and filed proper issues for each of the items you suggested. Please feel free to take any of them you would like. All help is much appreciated!

Thanks again!