Merge config, rest and realtime into one ably package #13
Conversation
|
Well I guess we must embrace the single package philosophy... 😢 |
| @@ -40,8 +40,8 @@ type PaginatedResource struct { | |||
| } | |||
|
|
|||
| // NewPaginatedResource returns a new instance of PaginatedResource | |||
| // It needs to be a struct implementing ResourceReader which in our case is rest.Client. | |||
| func NewPaginatedResource(typ reflect.Type, path string, params *config.PaginateParams, | |||
| // It needs to be a struct implementing ResourceReader which in our case is rest.RestClient. | |||
There was a problem hiding this comment.
The comment still mention rest as the package name.
There was a problem hiding this comment.
Done.
It mentioned also non-existing ResourceReader interface. The newPaginatedResource is unexported now, so I've removed the doc.
|
Can you also update the README to reflect that change? |
|
@rjeczalik I assume this style of putting everything in the same namespace is more normal for Go? |
This CL:
- moves config.Params to ably.Params
- moves proto.PaginatedResource to ably.PaginatedResource
- moves {rest,realtime}.Client to ably.{Rest,Realtime}Client to remove
circular deps on existing ably.New{Rest,Realtime}Client functions
|
@kouno @mattheworiordan Please take a look.
There's no rule nor practice of putting everything in one package no matter what. The normal for Go is to structure the packages around functionalities - functionalities in ably-go depends on each other, and with four separate packages it was a bit hard to keep the API clean. This single-package API is cleaner, however there's no issue revisiting it after it's fully complete. |
Ahah, once again a victory for @lmars who wrote it in a similar way before I jumped on the project. I hate when this guy is right ;). |
|
Actually, I said nothing. Seems like @lmars did create the first rest/realtime package too! /~https://github.com/ably/ably-go/tree/90c6d6341c595270608df67b2622400de63e6359 🎊 🎆 |
| Name: name, | ||
| client: client, | ||
| } | ||
|
|
||
| c.Presence = &Presence{ | ||
| c.Presence = &RestPresence{ |
There was a problem hiding this comment.
Can you add to the other issue that Presence needs to be a getter (presence()). This is to restrict the access to the Presence object.
There was a problem hiding this comment.
Aleady done, see second bullet here #15.
Merge config, rest and realtime into one ably package
|
@kouno yep I originally had two packages because there were two clients, and there was a straight forward dependency from Feel free to mention me in other PRs if you want another opinion 😄. |
This CL:
circular deps on existing ably.New{Rest,Realtime}Client functions