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

Generator options #98

Closed
wants to merge 2 commits into from
Closed

Generator options #98

wants to merge 2 commits into from

Conversation

mlesar
Copy link

@mlesar mlesar commented Feb 25, 2022

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

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
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
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.

@@ -39,7 +39,7 @@ import (
// UUID epoch (October 15, 1582) and Unix epoch (January 1, 1970).
const epochStart = 122192928000000000

type epochFunc func() time.Time
type EpochFunc func() time.Time
Copy link
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
Copy link
Collaborator

cameracker commented Jan 6, 2023

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