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

Unexported types make it more difficult to create interfaces for methods #179

Closed
erutherford opened this issue Oct 22, 2018 · 11 comments · Fixed by #633
Closed

Unexported types make it more difficult to create interfaces for methods #179

erutherford opened this issue Oct 22, 2018 · 11 comments · Fixed by #633
Labels
help wanted needs triage Ticket that needs triage (a proper look for classification)

Comments

@erutherford
Copy link

First I'd like to thank you for writing this package.

We were recently updating our package and noticed that there's an un-exported Type on the UserService Find method. While we can use the provided With* methods to provide these types, it breaks the ability to directly implement an interface for that method. For now we'll be anchoring at a version before this was added, but I ask that you reconsider this design decision.

@rbriski
Copy link
Contributor

rbriski commented Oct 22, 2018

Can you give an example of the way that you would like to access that function? I think this is a fairly recent addition so it hasn't gotten a ton of exercise yet.

Also, you might be giving us too much credit for long term design decisions. If you have a more idiomatic way to do this, I'm open to it.

@erutherford
Copy link
Author

erutherford commented Oct 22, 2018

Sure @rbriski , we're constructing a new client struct with only the methods we need to complete a specific task, this helps with mocking for tests and provides a cleaner interface only providing what we need access to.

// JiraClient is the client interface for making requests to jira
type JiraClient struct {
	Authentication AuthenticationProvider
	Issue          IssueProvider
	User           UserProvider
}

// AuthenticationProvider provides method to verify username and password
type AuthenticationProvider interface {
	AcquireSessionCookie(username, password string) (bool, error)
}

// IssueProvider provides method to create a new jira issue
type IssueProvider interface {
	Create(issue *jira.Issue) (*jira.Issue, *jira.Response, error)
}

// UserProvider provides method to find a jira user
type UserProvider interface {
	Find(name string) ([]jira.User, *jira.Response, error)
}

Now that there's an un-exported type we're unable to implement the interface. I'm not sure what the reasoning was behind not exporting those Types, but it seems like it might be beneficial for people to be able to provide a filter method as well.

@erutherford erutherford changed the title Unexported types are make it more difficult to create interfaces for methods Unexported types make it more difficult to create interfaces for methods Oct 26, 2018
@ghostsquad
Copy link
Contributor

I think this whole method needs refactoring:
https://github.com/andygrunwald/go-jira/blob/master/user.go#L182-L210

  1. The query parameters & url should be constructed using the stdlib URL package.
  2. I see now that the ability to filter should be exposed (there's really no need to keep it private)

@ghostsquad
Copy link
Contributor

@erutherford we are accepting PRs! :)

@github-actions
Copy link

github-actions bot commented May 3, 2020

This issue has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label May 3, 2020
@andygrunwald
Copy link
Owner

/unstale

I agree with everything that was said here.
If someone wants to pick this up, happy to accept PRs.

@andygrunwald andygrunwald removed the stale label May 3, 2020
@github-actions
Copy link

github-actions bot commented Jul 3, 2020

This issue has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jul 3, 2020
@ghostsquad ghostsquad removed the stale label Jul 3, 2020
@andygrunwald andygrunwald added needs triage Ticket that needs triage (a proper look for classification) and removed enhancement labels Aug 20, 2022
@andygrunwald
Copy link
Owner

Hey,

I am very sorry that this issue has been open for a long time with no final solution. We work on this project in our spare time, and sometimes, other priorities take over. This is the typical open source dilemma.

However, there is news: We are kicking off v2 of this library 🚀

To provide visibility, we created the Road to v2 Milestone and calling for your feedback in #489

The development will take some time; however, I hope you can benefit from the changes.
If you seek priority development for your issue + you like to sponsor it, please contact me.

What does this mean for my issue?

We will work on this issue indirectly.
This means that during the development phase, we aim to tackle it.
Maybe in a different way like it is currently handled.
Please understand that this will take a while because we are running this in our spare time.

Final words

Thanks for using this library.
If there is anything else you would like to tell us, let us know!

@AlexVulaj
Copy link
Contributor

@andygrunwald @ghostsquad Hey there - I've run into this issue myself just now and I'm happy to make this change and submit a PR.

I see that the main branch already contains v2 changes - is there a specific release branch or tagged release commit you'd like me to branch off of to submit the change for the 1.x release stream so this can get out there quickly? It's currently blocking my ability to write tests for my code so I'm interested in helping however I can.

Thanks!

  • Alex

@ghostsquad
Copy link
Contributor

@AlexVulaj please submit a PR, any backwards compatible changes are A-OK before V2, so a method that was previously unexported becoming exported should be fine.

@AlexVulaj
Copy link
Contributor

@ghostsquad good to know! I've submitted a PR linked above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted needs triage Ticket that needs triage (a proper look for classification)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants