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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

TJM
Copy link
Contributor

@TJM TJM commented Nov 2, 2020

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 TJM mentioned this pull request Nov 2, 2020
@TJM
Copy link
Contributor Author

TJM commented Nov 2, 2020

NOTE: This would replace PR #65

@TJM
Copy link
Contributor Author

TJM commented Nov 4, 2020

Fixes #67

@TJM
Copy link
Contributor Author

TJM commented Nov 16, 2020

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

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
@adlio
Copy link
Owner

adlio commented Mar 28, 2021

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

@adlio
Copy link
Owner

adlio commented Mar 29, 2021

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
Copy link
Contributor Author

TJM commented Mar 29, 2021

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.

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.

2 participants