Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define smaller interfaces for methods in adjudcator #185

Conversation

manoranjith
Copy link
Contributor

  • Previously, the adjudicator interface consisted of four methods.

  • But, only some of the methods were required in any particular place.

  • Hence, define an interface for each of the methods and compose the
    adjudicator interfaces from these smaller interfaces.

Resolves #184.

Copy link
Contributor

@matthiasgeihs matthiasgeihs left a comment

Choose a reason for hiding this comment

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

Some comments on the interface definitions. Let's discuss during our call later.

I have also some comments on the documentation, but we can do that afterwards.

channel/adjudicator.go Outdated Show resolved Hide resolved
channel/adjudicator.go Outdated Show resolved Hide resolved
@manoranjith manoranjith force-pushed the 184-smaller-interfaces-from-adjudicator branch 3 times, most recently from 13ae65f to 6f23552 Compare September 13, 2021 13:04
- Previously, the adjudicator interface consisted of four methods.

- But, only some of the methods were required in any particular place.

- Hence, define an interface for each of the methods and compose the
  adjudicator interfaces from these smaller interfaces.

Signed-off-by: Manoranjith <[email protected]>
@manoranjith manoranjith force-pushed the 184-smaller-interfaces-from-adjudicator branch from 6f23552 to 98997b5 Compare September 13, 2021 13:05
@manoranjith
Copy link
Contributor Author

Some comments on the interface definitions. Let's discuss during our call later.

I have also some comments on the documentation, but we can do that afterwards.

@matthiasgeihs As discussed in the call, I have revised the docs comments on the interfaces to idiomatic style (as used in the go standard library for for interfaces like io.Writer, io.WriteSeeker etc.,)

Also, I have removed the RegisterSubscriber method, which can be added as the part of the PR which uses that interface.

@matthiasgeihs
Copy link
Contributor

matthiasgeihs commented Sep 15, 2021

OK, so far this looks fine. However, I would like to wait with approval and merging until we have a complete draft of the implementation of the local watcher. I am not super happy about breaking up the adjudicator interface into many single-method interfaces. It might make sense to only separate those which are relevant to the watcher.

@matthiasgeihs
Copy link
Contributor

Closed in favor of #212

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define smaller interfaces for methods in Adjudicator
2 participants