andygrunwald / go-jira

Go client library for Atlassian Jira
https://pkg.go.dev/github.com/andygrunwald/go-jira?tab=doc
MIT License
1.48k stars 470 forks source link

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

Closed erutherford closed 11 months ago

erutherford commented 6 years ago

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

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

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.

ghostsquad commented 5 years ago

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

@erutherford we are accepting PRs! :)

github-actions[bot] commented 4 years ago

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.

andygrunwald commented 4 years ago

/unstale

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

github-actions[bot] commented 4 years ago

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.

andygrunwald commented 2 years ago

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 https://github.com/andygrunwald/go-jira/issues/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 commented 1 year ago

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

ghostsquad commented 1 year ago

@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 commented 1 year ago

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