Qualifiers Ordering

[tommyknows]

Hi again!

After #52, there seems to be a "bug" about the ordering of the qualifiers.
The code says:

// Qualifiers is a slice of key=value pairs, with order preserved as it appears
// in the package URL.

However, with the switch to the url.URL type, when encoding the qualifiers they actually get ordered lexicographically.

Now I've started to wonder what the "correct" behaviour for this is. The PURL Spec even says:

If the qualifiers are not empty and not composed only of key/value pairs where the value is empty:
...

• sort this list of qualifier strings lexicographically
  ...

So admittedly, according to the spec, the "new" behaviour is correct.
Should I submit a PR to remove the comment that the order is being preserved? I'm wondering if this should be considered a breaking change.

Or do you have an opinion / better idea on what to do here?

Thanks!

[tommyknows]

I'm just piling another small issue on top: #52 starts making use of JoinPath, which I've just noticed has only been added with Go 1.19. The go.mod file still only requires Go 1.17, so this just broke a build for me where a tool is using this library with Go 1.18...

I'd suggest to bump that version as well, or replace the usage of JoinPath with it's implementation:

	url, err := Parse(base)
	if err != nil {
		return
	}
	result = url.JoinPath(elem...).String()
	return

Again, happy to open a PR for both of these issues!

[shibumi]

Should I submit a PR to remove the comment that the order is being preserved? I'm wondering if this should be considered a breaking change.

Yes please. We should strictly follow the PURL spec and everything that differs, is clearly wrong.

[shibumi]

I'm just piling another small issue on top: #52 starts making use of JoinPath, which I've just noticed has only been added with Go 1.19. The go.mod file still only requires Go 1.17, so this just broke a build for me where a tool is using this library with Go 1.18...

We should only support the last 2 Go versions and maybe adapt our CICD that it tests the last two Go releases.

[shibumi]

PRs are welcome of course :)

[tommyknows]

Thanks for the quick reply! Agreed on both answers as well.

One question that follows is whether we should have a "custom" Qualifiers type at all, if we could just use url.Values instead?
I see a couple of options:

• Use url.Values directly
• Create an alias type, e.g. type Qualifiers = url.Values
• Keep what we have right now, and simply add some conversion funcs
  WDYT?

Note that apart form the last option, these would all be breaking changes - although admittedly they'd simply the API, for example removing the QualifiersFromMap function.

Disregarding breaking changes, I'd advocate for the second option as it still defines a clear "contract" of what a qualifier is.
But I'm fairly oblivious to the route chosen, and I can definitely see why we wouldn't want to introduce such "drastic" changes.

(edit: note that one downside of such an alias type is that we couldn't define our own methods on it)

[tommyknows]

hey @shibumi, quick ping in case you missed this :)

[shibumi]

Hey @tommyknows sorry for the delay. I am just out of surgery and still recovering :)

Honestly, I do not care as long as we are following the official PURL spec. Using url.Values directly or a type alias, seems to be the better long term solution.

For this issue in particular, I would also be fine with simply adding some conversion funcs, but using url.Values or a type alias is really something we should aim for.

If we go for url.Values or a type alias we will have to bump the major version to communicate the breaking API change correctly. Do you think it would be a lot of work to fix the bug first and then fully migrate to url.Values or type aliases later?

I'll leave that decision up to you :)

[tommyknows]

Oh sorry, I hope your recovery is going well! 🍀

Alright, I've opened #56 for this.