search

adlio / trello

Trello API wrapper for Go
MIT License
219 stars 71 forks source link

Use type.setClient(c) for consistency and ... #77

Open TJM opened 3 years ago

TJM commented 3 years ago

There are several Type(s) that have sub-types, and it was inconsistent. The goal here is to use type.setClient(c) every time, and if there are sub-types, loop through them and use their subType.setClient(c). Additionally, when possible, set the parent object. (i.e.) Board <-> List <-> Card <-> CheckList <-> CheckItem

Used the _, item for range, as the code is much cleaner. (and maybe easier to understand).

Also, minor "lint" change on board_test.go

TJM commented 3 years ago

NOTE: This would replace PR #65

TJM commented 3 years ago

Fixes #67

TJM commented 3 years ago

I will need to re-work this after the other changes

adlio commented 3 years ago

Apologies for the (long) delay.. been busy with work, but I'm going to be adjusting this to integrate with a new release.

adlio commented 3 years ago

This PR was addressing two separate issues:

  1. The .client property was inconsistently sideloaded
  2. Client.GetCard(), and similar Client.GetLowLevelThing() had a nil List or Board field (this is where the caching becomes useful).

The first was addressed in PR #84, in the context of Issue #82, which describes a need to use some of the low-level entity methods, without having to fetch the unneeded parent objects (e.g. you should be able to call Card.Delete() without having to fetch the Card struct from the server).

The new behavior allows manually calling SetClient() on all entities. When called on a high-level entity like Board or List, the call moves downward to the children (Cards on Lists, Lists on Boards, etc).

The second issue is still open. I'd like to implement something along these lines, and think a cache is appropriate, but there are some specific details about tying the cache to SetClient() that feel prone to surprise developers. The current code maps pretty closely to the API, so that most function calls result in a fairly obvious and predictable underlying API call. This PR would mean calling GetCard() would now result in 3 API calls (1 for the Card, List and Board).

One approach that appeals to me is adding GetList() and GetBoard() methods on Card{}, which would leverage some of the sideloading... so if you had a Card{} that came from a List, then card.GetList() would not need to make an API call... but if card.List was nil, it'd do the necessary API call and populate the .List field.

TJM commented 3 years ago

wow, blast from the past. I worked on this while I was staying in my dad's hospital room after a back surgery... don't have any time to dive into it right now, but I will review the changes when I have a chance.