High level client API overview #7

rsjethani · 2023-07-02T15:24:08Z

Here we create some models and types to define how the overall API would look like going forward. We simply establish how the api is supposed to be used by consumer.

README now show examples of client usage.

rsjethani · 2023-07-02T15:35:57Z

@StefMa / @chb-ioki once we establish this then adding api methods filling in other blanks would be an organic change on the API.

StefMa

Nice work so far 👏
Thanks for the initial work for this.
I have a few questions and recommendations 🙂

StefMa · 2023-07-02T18:11:54Z

README.md

+# Usage
+
+


Suggested change

ahh! I ususally keep more than one empty line between headings so as to differentiate with paragraphs of one section which are also separated by one line. looks a bit more clean in text-view. But anyways I will get rid of them.

StefMa · 2023-07-02T18:12:10Z

README.md

Suggested change

StefMa · 2023-07-02T18:17:00Z

README.md

+		return true
+	})
+}


I guess this is enough 🙃

Suggested change

fmt.Println(d)

return true

})

}

fmt.Println(d)

return true

})

StefMa · 2023-07-02T18:18:53Z

README.md

+Getting a single document by its id:
+```golang
+cl := outline.New()


cl? What about ol (for outline) or client... I don't like those abbreviations 😅

Identifier names should be proportional to their scope. The farther they are used from declaration the more pronounced their names should be. But here IMO the name is apt as per usage scenario. This how you will find var names in most go codebase 😆 I suggest: https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_choose_identifiers_for_clarity_not_brevity

StefMa · 2023-07-02T18:20:25Z

client.go

+// Documents creates client for operating on documents.
+func (cl *Client) Documents() *DocumentsClient { return nil }
+
+// Documents creates client for operating on collections.


Suggested change

// Documents creates client for operating on collections.

// Collections creates client for operating on collections.

good catch, thanks!

StefMa · 2023-07-02T18:20:46Z

collections.go

@@ -0,0 +1,3 @@
+package outline
+
+type CollectionsClient struct{}


documentation is missing 🙃

StefMa · 2023-07-02T18:21:39Z

models.go

+	DocumentID      string
+	DocumentShareID string
+	DocumentUrlID   string
+	CollecionID     string


Woooza, this is nice! 😲

CollecionID -> typo :)

StefMa · 2023-07-02T18:25:43Z

documents.go

+// Do makes the actual request and retrieves selected documents. The user provided callback fn is called for every such
+// document. If there is any error during the process then fn is given the error so that it can decide whether to
+// continue or not. The callback can return false in case it wants to abort getting documents.
+func (cl *DocumentsClientGetAll) Do(ctx context.Context, fn func(*Document, error) bool) error {


We have kinda the same public functions but different signatures.
Here we have a function to retrieve "the document".
In the other Do function, we return "the document".

I understand why we does this differently.
However, I think it would be easier to have the same signatures for similar/same functions.
So what about having a function in the other Do as well? (instead of return the document)?

Well the difference is intentional. Getting a single document is simple non-iterative process represented by a simpler method signature. Making that signature unnecessarily complex just to look like the other is not a good idea IMHO. And its not like the two calls are completely different rather they have same initial parameter sub-set which seems okay,

StefMa · 2023-07-02T18:26:14Z

client.go

+// Client is per server top level client which acts as entry point and stores common configuration (like base url) for
+// resource level clients. It is preferred to reuse same client while communicating to the same server as this makes
+// better utilization of resources.
+type Client struct{}


Do we want to introduce a sub-package for all this client/api related communication? 🤔

Do you mean where we put actual HTTP communication code? If yes then yes that logic will not be part of this type.

The goal of this project is to have an cli tool, right? That we do some internal http stuff, so that it works, are internal details that shouldn't be exposed to cli users (kinda), right?
And this client is a "wrapper" around a http library that will be used inside the cli only, right?

However, I guess it make sense to schedule a meeting to discuss this 🙃

Clarified in call.
All fine now ✔️

StefMa · 2023-07-02T18:28:00Z

README.md

+
+
+## Client


The goal of this project is to be an CLI.
Now we document the (http) client we use exclusively internally.
Does it make sense to document this somewhere else (or even not at all 😅 ) instead of the README?

However, I really appreciate the documentation. It helped me a lot!

Clarified in call.
All fine now ✔️

StefMa · 2023-07-03T06:47:54Z

To solve #5 we also need a "publish document" function.

rsjethani · 2023-07-03T08:36:32Z

@StefMa I added #8 to answer some questions here. Also this PR is not meant to add any functionality it just wants to establish API usage pattern that is it. There are of course lot of this missing :) and nothing is 'polished'

StefMa

LGTM for now 👌 🙂

Here we create some models and types to define how the overall API would look like going forward. We simply establish how the api is supposed to be used by consumer. README now show examples of client usage.

rsjethani · 2023-07-06T20:23:47Z

@chb-ioki please approve if all looks good to you, thanks!

StefMa · 2023-07-07T08:13:19Z

@rsjethani I guess we are good to go now 🙃
Feel free to merge. (Same applies for the other PR #9)

chb-ioki

Nice!

I really like the interface! cl.Documents().Get().ByID("document id")

rsjethani marked this pull request as ready for review July 2, 2023 15:24

StefMa reviewed Jul 2, 2023

View reviewed changes

rsjethani requested a review from StefMa July 3, 2023 09:19

StefMa approved these changes Jul 3, 2023

View reviewed changes

rsjethani requested a review from chb-ioki July 3, 2023 13:57

StefMa mentioned this pull request Jul 3, 2023

Acutal HTTP request making infrastructure #9

Merged

rsjethani self-assigned this Jul 6, 2023

rsjethani added 3 commits July 6, 2023 22:22

High level client API overview

e540209

Here we create some models and types to define how the overall API would look like going forward. We simply establish how the api is supposed to be used by consumer. README now show examples of client usage.

review changes

d1a83c1

fix typo

5c54a54

chb-ioki approved these changes Jul 7, 2023

View reviewed changes

StefMa merged commit fd82ee7 into ioki-mobility:main Jul 7, 2023

rsjethani deleted the client-api-overview branch July 7, 2023 10:11

rsjethani added this to the 0.1.0 release milestone Jul 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

High level client API overview #7

High level client API overview #7

rsjethani commented Jul 2, 2023

rsjethani commented Jul 2, 2023 •

edited

Loading

StefMa left a comment

StefMa Jul 2, 2023

rsjethani Jul 3, 2023

StefMa Jul 2, 2023

StefMa Jul 2, 2023

StefMa Jul 2, 2023

rsjethani Jul 3, 2023

StefMa Jul 2, 2023

rsjethani Jul 3, 2023

StefMa Jul 2, 2023

StefMa Jul 2, 2023

chb-ioki Jul 3, 2023

StefMa Jul 2, 2023

rsjethani Jul 3, 2023

StefMa Jul 2, 2023

rsjethani Jul 3, 2023

StefMa Jul 3, 2023

StefMa Jul 4, 2023

StefMa Jul 2, 2023

StefMa Jul 4, 2023

StefMa commented Jul 3, 2023

rsjethani commented Jul 3, 2023

StefMa left a comment

rsjethani commented Jul 6, 2023

StefMa commented Jul 7, 2023

chb-ioki left a comment

	// Documents creates client for operating on collections.
	// Collections creates client for operating on collections.

		@@ -0,0 +1,3 @@
		package outline

		type CollectionsClient struct{}

High level client API overview #7

High level client API overview #7

Conversation

rsjethani commented Jul 2, 2023

rsjethani commented Jul 2, 2023 • edited Loading

StefMa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefMa commented Jul 3, 2023

rsjethani commented Jul 3, 2023

StefMa left a comment

Choose a reason for hiding this comment

rsjethani commented Jul 6, 2023

StefMa commented Jul 7, 2023

chb-ioki left a comment

Choose a reason for hiding this comment

rsjethani commented Jul 2, 2023 •

edited

Loading