Initial implementation

[JPEWdev]

The initial implementation of the SPDX go bindings

[mengzhuo]

I think we need to review the "import path" of this mod/package while user will confused by the version between spec and go module itself.

It's better to use multiple modules
Ref: https://go.dev/doc/modules/managing-source#multiple-module-source

spdx-v3 github.com/spdx/spdx-go-model/spdx-v3-0-1
spdx-v4 github.com/spdx/spdx-go-model/spdx-v4-0-0

While user can install by

go get github.com/spdx/spdx-go-model/[email protected]
go get github.com/spdx/spdx-go-model/spdx-v4-0-0@latest

or we can setup a prettier path like k8s project (k8s.io/api/core/v1) as for SPDX like spdx.dev/go/v3.0.1@latest

[JPEWdev]

I'm not sure we actually want to do multiple modules in this case. If we make multiple modules in the same repository, it looks like when we release, we would have to drop a tag for each version of SPDX spec in the repo. For example, to make a release, we would have to make tags:

spdx_v_3_0_1/v1.0.0
spdx_v_3_1_0/v1.0.0

etc.

(note: I couldn't figure out how to a module with - in the name)

Which will be a real pain given that we'll hopefully have several simultaneous SPDX versions in here, and also because all of the tags will always be dropped in the same location (because, all the code in this repo is effectively auto generated each release).

Given that the bindings for each release will "behave" pretty much the same for each SPDX version, I'm also not convinced that you'd want to allow users to pick different releases of the different bindings (e.g. v1.0.0 bindings for SPDX 3.0.1, v1.0.1 for SPDX 3.1.0, etc.). The binding code is only going to drastically change if we bump the version of shalc2code used to generate the bindings and I think we can manage that by A) fixing the version of shacl2code used (which I will do), and B) bumping the module major version and branching if necessary (e.g. when shacl2code changes the API).

[JPEWdev]

Also of note, I will make the bindings be named spdx_v3_0_1 instead of just v3_0_1 as that does remove the need for the import alias

[goneall]

I only reviewed the README, workflows and related artifacts - which all look good.

Looking for others with more GoLang expertise to review the generated code.

[viveksahu26]

@JPEWdev could you please give me a overview here about what you were looking a review for. Yesterday, in the meeting there were some glitch in the network from my side due to which I couldn't understand fully.

[JPEWdev]

@JPEWdev could you please give me a overview here about what you were looking a review for. Yesterday, in the meeting there were some glitch in the network from my side due to which I couldn't understand fully.

I'm not a go expert, this is the first go program I've ever written, so what I would like is for some people knowledgeable in go to look over the code (in particular the usage in the examples) and make sure it's reasonable from a go user perspective

[viveksahu26]

Use append() for Slices Instead of Custom Methods.

• Instead of ci.CreatedBy().AppendObj(p), use ci.CreatedBy = append(ci.CreatedBy, p).

The built-in append function is the idiomatic way to work with slices in Go.

[viveksahu26]

Avoid Unnecessary Getters

• Instead of using p.ID().Set(value), use p.SetID(value),
• Similarly, p.Name().Set(value), use p.SetName(value)
• Similarly, ci.SpecVersion().Set(value), use ci.SetSpecVersion(value)
• and, ci.Created().Set(valu), use ci.SetCreated(value)

Something like this: https://go.dev/play/p/tREj7_5eQFN

You can checkout these resources to understand more about getter and setter in go:

• https://stackoverflow.com/questions/11810218/how-to-set-and-get-fields-in-structs-method/11842484#11842484
• https://softwareengineering.stackexchange.com/questions/278807/does-go-implicitly-discourage-getters-and-setters-and-encourage-direct-member-ac

[viveksahu26]

Similarly here: Avoid Unnecessary Getter/Setter Chaining:

Refractor it to:

email.SetExternalIdentifierType(spdx_v3_0_1.ExternalIdentifierTypeEmail)
email.SetIdentifier("[email protected]")

[viveksahu26]

Similarly avoid AppendObj and simply refractor it to:

p.ExternalIdentifiers = append(p.ExternalIdentifiers, email)

[viveksahu26]

If this, if it is internally appending an objects as name suggest, AddObject, then instead of that use a simple append().
Refractor it as:

objset.Objects = append(objset.Objects, p, doc)

[viveksahu26]

Similarly these one, as per above suggested to avoid getter.

• Instead of doc.CreationInfo.Set(value), use doc.SetCreationInfo(ci), and
• doc.ID().Set(value), by doc.SetID(value)

Refractor with:

func (d *SpdxDocument) SetCreationInfo(ci CreationInfo) {
    d.CreationInfo = ci
}

and

func (d *SpdxDocument) SetID(id string) {
    d.SetID = id
}

[viveksahu26]

Ensures test execution stops immediately when a failure occurs.

• t.Error(err) will continue test even after errors
• whereas, t.Fatal(err), will stops execution test upon failure.

So, refractor this too:

if err != nil {
    t.Fatal(err) // Terminates test immediately
}