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

Feat/service interfaces #14

Closed
wants to merge 5 commits into from
Closed

Feat/service interfaces #14

wants to merge 5 commits into from

Conversation

ciroque
Copy link

@ciroque ciroque commented Sep 30, 2019

Addresses Issue #13

Uses the go generate command to run vburenin's ifacemaker that generates files containing interfaces based on the structs.

Also includes a script that writes the go:generate comment to each of the service files.

The general idea is that users of the library can now create mocks of the various Services. This ensures testing boundaries within a given code base coincide with the use of the library. That is, tests in user's code test only their code and can provide success / failure scenarios by mocking the required service and focus on the "Class Under Test".

@ciroque
Copy link
Author

ciroque commented Sep 30, 2019

Hi @hbagdi ,

I was able to carve out some time to work on this. Finding the ifacemaker library was great. Made this pretty straightforward to accomplish.

Let me know if you would like me to change anything about the way this is done.

Thanks!

r/Steve

@hbagdi
Copy link
Owner

hbagdi commented Sep 30, 2019

I had a brief look at this but I'm liking it so far! Nice job!

One key thing that is missing is to have a script to verify if the code-gen is up-to-date or not.
And then also integrate that with CI so that it can catch when a method is added to a service but not to the corresponding Interface.

@ciroque
Copy link
Author

ciroque commented Oct 1, 2019

Would it be enough to run go generate in the travis.yml, like this:

before_install:
  - go generate ./kong

(I have not tested that yet)

@ciroque
Copy link
Author

ciroque commented Oct 1, 2019

Oh interesting. I found this command that would fail the build if the interfaces were out of sync:

go generate -x ./... && git diff --exit-code; code=$?; git checkout -- .; (exit $code)

Lifted from: https://github.com/google/go-github/blob/master/.travis.yml#L24

It would not automatically generate the new interfaces, but would kick back to the dev to run go generate and commit the interface changes...

cool?

@hbagdi
Copy link
Owner

hbagdi commented Oct 6, 2019

Yes, that would work.
All we need is to ensure that CI fails a commit if there is a function added onto the struct but it is not added in the interface.

Copy link
Owner

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

Please make sure that CI is running and detecting the changes.

@@ -25,3 +25,4 @@ _testmain.go
*.prof

# for this repo only
/.idea/
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this.
You can add this to your global gitignore file:
https://davidwalsh.name/global-gitignore

@@ -0,0 +1,45 @@
#!/bin/bash

extractStructName() {
Copy link
Owner

Choose a reason for hiding this comment

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

Where is this script being used?

@@ -1,5 +1,7 @@
package kong

//go:generate ifacemaker -f acl_service.go -s ACLService -i ACLServiceInterface -p kong -c "DO NOT EDIT: Auto generated" -o acl_service_interface.go

Copy link
Owner

Choose a reason for hiding this comment

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

This is little bit of nitpick but I think it's important.
Can we move this just above the the declaration of the service?
This helps when someone is reading only part of the code or duplicating this into another service.

This comment applies to all the generate comments.

@hbagdi
Copy link
Owner

hbagdi commented Oct 17, 2019

@ciroque Friendly ping. This is almost there.

@hbagdi
Copy link
Owner

hbagdi commented Dec 4, 2019

@ciroque Friendly ping.

@hbagdi
Copy link
Owner

hbagdi commented Dec 31, 2019

@ciroque Another friendly ping. :)

@hbagdi
Copy link
Owner

hbagdi commented May 15, 2020

This would have been an amazing addition to the project. If someone else wants to take this up, please feel free!

@hbagdi hbagdi closed this May 20, 2020
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.

2 participants