openvex / go-vex

Go module to generate and transform VEX documents
Apache License 2.0
33 stars 15 forks source link

Consider renaming `csaf.CSAF` to `csaf.Document` #8

Closed picatz closed 11 months ago

picatz commented 1 year ago

This is a subjective thing, but I'll present the idea anyway. I think it might make sense to rename csaf.CSAF to csaf.Document to avoid repetition. This type "stuttering" is sometimes considered an anti-pattern that is (sadly) found in a few places inside the standard library (like context.Context).

https://github.com/openvex/go-vex/blob/2d2b6ef7e805a1babe18f2490946d4b3e835ce00/pkg/csaf/csaf.go#L10

An added benefit is that csaf.Document would be more descriptive to that it actually is, information from a "CSAF document". To avoid worries about a breaking change, we could add a type alias.

What do you all think?

picatz commented 1 year ago

A similar anti-pattern exists in the vex package. It might make sense to rename vex.VEX to vex.Document:

https://github.com/openvex/go-vex/blob/2d2b6ef7e805a1babe18f2490946d4b3e835ce00/pkg/vex/vex.go#L49-L50

noah-de commented 1 year ago

Regarding this bit of code: https://github.com/openvex/go-vex/blob/abb7755097f670b05c5c31d44281629135056c97/pkg/csaf/csaf.go#L13 I would agree that a name change makes sense.

puerco commented 1 year ago

+1 It makes sense to me. I'll handle this soon. /assign

picatz commented 1 year ago

Awesome, thank you @puerco!

frenchi commented 11 months ago

Jumping in to this thread after the v0.2.0 spec introduced some potentially breaking changes that we reviewed over in the GUAC project. Thankfully, it was a non-issue, but it prompted me to go looking for any potential other future breaking changes, which this seems like this may be.

Looks like both the csaf and vex packages aren't too widely used yet, but I suspect this will become harder the longer we leave it. So I'd be happy to tackle this refactor now, if we decide that we still wanted to get it done (especially considering most of the public consumers seem to be us over in GUAC-land).

So, can I get a 👍 / 👎? is this: A.) Still a priority, and do you want assistance with PRs @puerco? or B.) Do we want to accept csaf.CSAF & vex.VEX as the public interfaces, even though the stutter isn't strictly idiomatic go?

I don't have a strong preference either way, aside from if we're making any interface changes, it's probably better to do them sooner rather than later.

picatz commented 11 months ago

FWIW, I hope we don't land on option B, but I understand if that ends up being the case.

If we want to avoid breaking changes, we can always use a type alias so that csaf.CSAF would be equivalent to a future csaf.Document type.

puerco commented 11 months ago

Sorry I pledged to fix this legacy type and didn't. And I think we have too many dependencies now to change it. We could alias it but we cannot change the functions anyway because we'd break a lot of people. So I think it will be B :/

picatz commented 11 months ago

While I'm honestly slightly disappointed that ended up being the result, I totally understand, and don't need to press on this issue future.