Skip to content

Generator options#98

Closed
mlesar wants to merge 2 commits into
gofrs:masterfrom
mlesar:gen-options
Closed

Generator options#98
mlesar wants to merge 2 commits into
gofrs:masterfrom
mlesar:gen-options

Conversation

@mlesar

@mlesar mlesar commented Feb 25, 2022

Copy link
Copy Markdown

Hi, I had to generate UUIDv6 with custom timestamps, so I added generator options to be able to change the epoch function.
I'm creating this PR in case this is something that others might need or find useful.

Bye,
Matija

Comment thread generator.go
return NewGenWithHWAF(defaultHWAddrFunc)
// NewGen returns a new instance of Gen with default values if no options are
// passed.
func NewGen(opts ...GenOption) *Gen {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As written this would be a breaking change for the module, and so I don't think we can accept this if it changes NewGen(). We should probably rename this to NewGenWithConfig(), and keep the NewGen() signature as-is.

Secondarily, I really dislike functional options and would prefer we do not adopt them for this use case. They are really hard to discover via documentation, where-as a configuration struct could be a lot more approachable / discoverable and still also support adding new options without it being a breaking change.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

As written this would be a breaking change for the module,

Why it would be a breaking change if you can call it as before uuid.NewGen()?

Secondarily, I really dislike functional options and would prefer we do not adopt them for this use case. They are really hard to discover via documentation, where-as a configuration struct could be a lot more approachable / discoverable and still also support adding new options without it being a breaking change.

Just to make sure I understand this, you would prefer to have it like this:

type GenConfig struct {
	EpochFunc  EpochFunc
	HWAddrFunc HWAddrFunc
	RandReader io.Reader
}

func NewGen() *Gen {
	return NewGenWithConfig(GenConfig{})
}

func NewGenWithConfig(config GenConfig) *Gen {
	if config.EpochFunc == nil {
		config.EpochFunc = time.Now
	}
	if config.HWAddrFunc == nil {
		config.HWAddrFunc = defaultHWAddrFunc
	}
	if config.RandReader == nil {
		config.RandReader = rand.Reader
	}

	return &Gen{
		epochFunc:  config.EpochFunc,
		hwAddrFunc: config.HWAddrFunc,
		rand:       config.RandReader,
	}
}

?

I find it more specific when using options, as you just use the one that you want to adjust, with config it seems as if someone has to set all of the properties.

Comment thread generator.go
const epochStart = 122192928000000000

type epochFunc func() time.Time
type EpochFunc func() time.Time

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that this is being exported, could you please add a GoDoc-compatible comment that explains what this is:

// EpochFunc is ...

@cameracker

cameracker commented Jan 6, 2023

Copy link
Copy Markdown
Collaborator

Closing this PR as it appears to have been resumed by another contributor in #111

@mlesar , thanks for laying the groundwork for this new pr.

@cameracker cameracker closed this Jan 6, 2023
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.

3 participants