Skip to content

Conversation

smacker
Copy link
Contributor

@smacker smacker commented Apr 8, 2019

Lookout contains convenient wrapper for grpc data client.
It's meant to be used by analyzers, so only dummy analyzer uses it
currently.

This commit moves the wrapper into lookout-sdk to allow all analyzers
use it.

I put it into new sdk package which is higher level sdk for Go than
current pb. We also may reconsider which custom code belongs to pb and
move the rest into this package to separate low level auto generated
code from actual sdk API.

smacker added 3 commits April 8, 2019 12:10
Lookout contains convenient wrapper for grpc data client.
It's meant to be used by analyzers, so only dummy analyzer uses it
currently.

This commit moves the wrapper into lookout-sdk to allow all analyzers
use it.

I put it into new `sdk` package which is higher level sdk for Go than
current pb. We also may reconsider which custom code belongs to pb and
move the rest into this package to separate low level auto generated
code from actual sdk API.

Signed-off-by: Maxim Sukharev <[email protected]>
Signed-off-by: Maxim Sukharev <[email protected]>
@smacker smacker requested a review from a team April 8, 2019 10:11
@se7entyse7en
Copy link
Contributor

We also may reconsider which custom code belongs to pb and
move the rest into this package to separate low level auto generated
code from actual sdk API.

I think that's a good idea! 👍

Copy link
Contributor

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! thanks!

@carlosms
Copy link
Contributor

carlosms commented Apr 9, 2019

LGTM about moving the code.

Before we merge this, I have a question about the destination folder.
In master we have:

  • ./python for all the python SDK code
  • ./pb for all the go SDK code

Now it will be:

  • ./python for all the python SDK code
  • ./pb and ./sdk for the go SDK code

Should we have instead ./go/pb and ./go/sdk?
I don't like breaking compatibility of the current imports, but I'm wondering if having the root folder of the go sdk ./ can bring more problems in the future, or be more inconvenient to use....

@se7entyse7en
Copy link
Contributor

I also prefer having ./python and ./go instead of having go sdk in ./. Currently, it seems that SDK for Python is some sort of second-class thing, but I don't think that this is the case (correct me if I'm wrong). So in the end we'll have:

  • proto - common definitions,
  • python - for python SDK,
  • go - for go SDK.

@smacker
Copy link
Contributor Author

smacker commented Apr 10, 2019

I think pb directory should be inside go.
I didn't move it because it's breaking change.
How about if I move sdk into go here and for pb we do another PR combined it with moving some code from pb to sdk?

@carlosms
Copy link
Contributor

sounds good to me

Signed-off-by: Maxim Sukharev <[email protected]>
@smacker smacker merged commit 2ca64a7 into src-d:master Apr 10, 2019
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.

4 participants