src-d / lookout-sdk

SDK for lookout analyzers
Apache License 2.0
4 stars 11 forks source link

move DataClient from lookout to new sdk package #88

Closed smacker closed 5 years ago

smacker commented 5 years ago

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.

se7entyse7en commented 5 years ago

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! 👍

carlosms commented 5 years ago

LGTM about moving the code.

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

Now it will be:

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 commented 5 years ago

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:

smacker commented 5 years ago

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 commented 5 years ago

sounds good to me