Skip to content

Conversation

@se7entyse7en
Copy link
Contributor

No description provided.

@se7entyse7en
Copy link
Contributor Author

From the next release I'd then revert this this so that by importing DataStub from pb it will use the new api.

@@ -0,0 +1,77 @@
#!/usr/bin/env python3
Copy link
Contributor

Choose a reason for hiding this comment

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

imo we don't need to include this example. But add a test that checks "normal" grpc code works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, gonna remove it. BTW how do you propose to test it? I mean the normal gRPC datastub should be guaranteed to work by gRPC itself, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't work right now on master. So we can break it again in the future occasionally.

I propose something like:

class DummyAnalyzer(pb.AnalyzerServicer):
    def NotifyReviewEvent(self, request, context):
        return pb.EventResponse(messages=["review"])

    def NotifyPushEvent(self, request, context):
        return pb.EventResponse(messages=["push"])


class Test:
   def test_review():
      res = stub.NotifyReviewEvent(event_pb2.ReviewEvent())
      self.assertEqual(res.messages, ["review"])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the tip! done in 8053a25.
I also moved some utility testing so that analyzers could eventually use the mixin for testing purposes.

if name.startswith("__"):
continue

default_func = dct.get(name, func)
Copy link
Contributor

Choose a reason for hiding this comment

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

I knew it's 1 line change!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I was ignoring if the method is actually defined 😅.

Signed-off-by: Lou Marvin Caraig <[email protected]>
Signed-off-by: Lou Marvin Caraig <[email protected]>
@se7entyse7en se7entyse7en force-pushed the analyzer-servicer-default-func branch from fb9df66 to 8053a25 Compare March 20, 2019 14:20
@se7entyse7en se7entyse7en merged commit a8da4eb into src-d:master Mar 20, 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.

3 participants