feat!: Introduce New function with functional options pattern

[jkroepke]

Today, we have separate functions for creating the Parser - NewFromSaved for using the built-in yaml definitions, NewFromBytes for providing your own regexes as bytes of a yaml file, and NewWithOptions for providing a path to a yaml file with various options from the library selected - e.g. configuring the LRU cache size, turning on the (unsafe) sorting mode, and options related to that.

This PR implements a New function that using the function options pattern. So instead of using any of the old interfaces, you can now New(WithRegexDefinitions(...)) to provide your own regexes, New(WithCacheSize(...)) to customize the cache size, or any combination of our new With* functions - the full list added here is: WithMode, WithDebug, WithSort, WithCacheSize, WithMissesThreshold, WithMatchIdxNotOk, and WithRegexDefinitions

[dgoldstein0]

So this looks simple enough that I think it's worth considering, though when I look around this file I see that it'd be annoying to specify your own parserConfig because the defaults are all hidden in this file - defaultParserConfig isn't public. One path forward is to change that, so a caller could call that, change the fields they want, then call your NewFromSavedWithOptions; another option would be to base the new function on NewWithOptions but using DefinitionYaml instead of having a regexFile string argument... which would basically be NewFromBytes but with the parser config options - and we could have a version to use the DefinitionYaml and another to pass your own yaml bytes if we wanted to have both.

So as written I don't think this PR quite makes sense, but there are several options to expose an interface.

[jkroepke]

Generally, I can recommend the functional options pattern: https://golang.cafe/blog/golang-functional-options-pattern.html

Maybe consider go:embed to store the default yaml file as []bytes and handle it as default option. This could unify the NewFromBytes/NewFromSaved code path as well.

I can offer a PR, if you wish.

[dgoldstein0]

apologies for the radio silence, been a very busy last few weeks and keeping up with maintenance here is something I try to fit into my spare time.

Generally, I can recommend the functional options pattern: https://golang.cafe/blog/golang-functional-options-pattern.html

This is an interesting idea, but one I'm not immediately sold on. A decade of python & JavaScript experience has made me wary of higher-ordered functions & callbacks, as I've seen codebases that lean into those too much evolve quite poorly over time. That said if this truly is common for golang maybe it's the right move - golang is definitely more of a second language to me so I'll ask some colleagues what they think. Really wish golang offered named parameters with default values like python & js as that would then be the obvious choice.

I'm also not quite sure what you have in mind in terms of that embed idea. One thing I see is it would help us reduce the build.sh logic if go's compiler would do the embedding for us, but it still wouldn't eliminate the fact that we have to manage a submodule, so I'm not quite sure what that would buy us. We probably could refactor to have the yaml definitions as a byte[] that can be overridden somehow without needing a new dependency.

[jkroepke]

We probably could refactor to have the yaml definitions as a byte[] that can be overridden somehow without needing a new dependency.

That is what I head in sense. If []byte is accepted, it doesn't matter, if the yaml comes from os.File, http or embed.

This is an interesting idea, but one I'm not immediately sold on.

What I say: I can observe, that this pattern is used across go libraries as well. I guess will be the golden path, unless we have named parameters or overloading as well.

Also see an reddit thread where the named pattern is used as answer to missing overload: https://www.reddit.com/r/golang/comments/19462z9/comment/khe039a/?utm_source=share&utm_medium=web3x&utm_name=web3xcss&utm_term=1&utm_content=share_button

[dgoldstein0]

I asked around and folks agree the functional options pattern is the best we can do in golang. I suppose to fully switch to it is a breaking change, but at least one that shouldn't change the libraries' behavior for folks that correctly migrate to the new interface. So I'd be willing to accept such a change. With review & appropriate documentation of course.

Do you want to make a new pr for that, or roll those changes into this pr?

Aside: I've worked with c++ before and it's overly broad abuse of overloading makes me happy that overloading has fallen out of favor as a language feature. Though to be fair it's way worse in c++ with implicit conversions.

[jkroepke]

Hi, I have updated the PR.

It's an breaking change now, but the New function looks very clean now.

[dgoldstein0]

Apologies for being slow to review this - as you might have guessed I'm maintaining this in my not-very-copious free time.

Main feedback:

• I think we shouldn't require DefinitionYaml to be explicitly passed to New, but rather that should also be the default and overridable with the option pattern.
• Can you update the description of this PR to explain the expected replacements for NewFromSaved, NewFromBytes, & New

I'll try to be quicker on the next iteration of review.

[dgoldstein0]

Looking good, but this currently does not compile (see the CI errors). Fix that and lets document the changes in the PR description (so they end up in the commit message) - I'm thinking a bunch of " should now be written as " would be really helpful to people who have to migrate.

[dgoldstein0]

still doesn't compile, still buggy per comments inline. It might be a good idea to try to run the tests per the readme instructions before updating again.

I appreciate bringing back the deprecated functions so that we give folks an easier migration pathway.

[jkroepke]

Also please check ua-parser/uap-core#640

[dgoldstein0]

one question, depending on the answer either I'll merge this as is or maybe with one more small change.

Also one of us should update the description before merging to describe the changes to the New function.

[dgoldstein0]

the more I look into this sort stuff the more things I notice about it.

[dgoldstein0]

just realized my last feedback was left hanging - my one hangup before merging this is that it's not really correct to have the MatchesCount fields shared across different Parser instances. That said those fields only matter when UseSort is true, so I'm thinking we should do a full, deep copy when instantiating the Parser with the UseSort option, and otherwise we can share the RegexDefinitions.

if you make that change I'll be happy to merge this PR.

[dgoldstein0]

one last feedback pass - if there's a benefit to these extra copies we can keep them, but I can't think of any (unless we were to try to use the copying to reduce the need for locking, but that's not what the code does, and is possibly harder to reason about).

[jkroepke]

you are right

[dgoldstein0]

Looks great! thanks for the contribution, this is a significant improvement to our interface.