Remove the WithOptions suffixes

[suhaibmujahid]

Is your feature request related to a problem? Please describe.

Many methods have the suffix WithOptions e.g., GetCreateMetaWithOptionsWithContext.

Describe the solution you'd like

The name could be replaced with simply GetCreateMeta and chick if the options is nil will use the default options. I think this approach also used in google/go-github.

[github-actions]

Hi! Thank you for taking the time to create your first issue! Really cool to see you here for the first time. Please give us a bit of time to review it.

[ghostsquad]

Really for v2 we should start using the functional options API approach

https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

[suhaibmujahid]

In the context of the article, the author examples expose the options as functions. In our context, I think struct is a better fit. The official MongoDB driver is a good example of this pattern mongodb/mongo-go-driver. However, our use case is simpler.

[ghostsquad]

@suhaibmujahid the functions take a struct as it's only parameter, so it's like passing in a struct, except you can differentiate between the zero-value of a field and whether or not the user passed it in.

The library can define "pre-baked" options, but the client can make their own and/or just pass in an anonymous function.

https://www.sohamkamani.com/golang/options-pattern/

[suhaibmujahid]

Thank you @ghostsquad for sharing the resources. I like this pattern, and it solve a use case for me and I will adopt it in my code.

But still I am not sure if it is an over fit for go-jira.

[ghostsquad]

@suhaibmujahid I'll need a bit more on why you are saying it's not a good fit. Some example code would be great. In an effort though to continue this conversation, here are some concrete examples from me, and further explanation.

Here's the current implemention of the function we are discussing

func (s *IssueService) GetCreateMetaWithOptionsWithContext(ctx context.Context, options *GetQueryOptions) (*CreateMetaInfo, *Response, error)

and here's GetQueryOptions struct

/ GetQueryOptions specifies the optional parameters for the Get Issue methods
type GetQueryOptions struct {
	// Fields is the list of fields to return for the issue. By default, all fields are returned.
	Fields string `url:"fields,omitempty"`
	Expand string `url:"expand,omitempty"`
	// Properties is the list of properties to return for the issue. By default no properties are returned.
	Properties string `url:"properties,omitempty"`
	// FieldsByKeys if true then fields in issues will be referenced by keys instead of ids
	FieldsByKeys  bool   `url:"fieldsByKeys,omitempty"`
	UpdateHistory bool   `url:"updateHistory,omitempty"`
	ProjectKeys   string `url:"projectKeys,omitempty"`
}

Right off the bat, we are faced with "boolean" options. It's nearly impossible for these options to have a default, or to even tell if it was set by the user or not.

Using functional options, defaults can be set to any value. And if none of the functional options change it, then it remains the default value. That default value can be the zero-value of the type, or something completely different. In the example below, it's not possible to default IsAwesome to true without using functional options.

Example

type FooOptions func(*Foo)

type Foo struct {
    IsAwesome boolean
}

func DoFoo(options ...FooOptions) {
    f := &Foo{
        IsAwesome: true
    }

    for _, o := range options {
        o(f)
    }
}

Here's one PR where we are testing to see if the option is set to 0 or not (the zero-value of an integer), to determine if it was default. It some cases, 0 is valid, so how would you see if it was set by the user?

6a34ec5

Here is another issue where we ran into how to figure out what default was.

#223

I've seen many other libraries simply just use string/bool pointers so they can detect whether it is nil or not and provide a default value. That way works, but I think you have to write a lot more guard code to ensure you don't get nil pointer panics.

The standard library also uses functional options in some places.

https://godoc.org/golang.org/x/text/secure/precis#Option
https://godoc.org/golang.org/x/text/cases#Option

Additionally, functional options allow you to set multiple fields at the same time, and to manipulate the given struct however you see fit. It's much more powerful than just taking a struct as a parameter.

Finally, the library itself can provide options (pre-baked functions) that the client can pass in, just like in the standard library examples.

Please help me understand what about this library makes a good design pattern a bad fit?

[suhaibmujahid]

Thank you @ghostsquad for the nice discussion. I'm convinced by the cases that you showed.

What I was thinking about, why I should write functions if I can just pass a struct (You answered that). Since we have cases such as having true as the default value, it makes sense to use this pattern.

I like about this pattern that merging multiple options happens out of the box without any extra complexity.

I'm voting toward the functional options pattern.

[andygrunwald]

Nice discussion. And thanks for the education on this pattern. I was not aware of this. However, I really like the approach.

Implementation wise this would lead to a lot of code to write. Especially when providing pre-defined functions. We might think about taking a generating approach to do the first heavy lifting. What do you think?

An alternative: Adding functions in an iterative approach. E.g. just starting with the function accepting options. And in every next release, we offer a few more functions. It would make this a long process, however, it will lower the burden of writing "somehow" repetitive code.

[ghostsquad]

@andygrunwald actually there's not a lot of work here at all. Here's how we would do it:

type GetQueryOptions struct {
   ...
}

type CreateMetaOption func(queryOption *GetQueryOptions)

func (s *IssueService) GetCreateMeta(ctx context.Context, options ...CreateMetaOption) (*CreateMetaInfo, *Response, error) {
   opts := &GetQueryOptions{
	  // define defaults here
   }

   for _, o := range options {
	 o(opts)
   }

   // use opts 
}

As you suggested, we don't need to define any "pre-baked" option functions for the end-user on the first pass, unless we already know of a common use case.

[github-actions]

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]

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!