Rename packages to improve readability.

[calebbrown]

This fixes #651

Changes:

• internal/pkgecosystem -> internal/pkgmanager
• pkg/api -> pkg/api/pkgecosystem + pkg/api/analysisrun.DynamicPhase
• pkg/result -> pkg/api/analysisrun
• pkg/notification -> pkg/api/notification
• pkg/pkgidentifier -> pkg/api/analysisrun.Key

Note - this is a breaking change for the AnalysisRunComplete notification

[maxfisher-g]

Nice changes overall! Just a few comments here and there.

[maxfisher-g]

Can we remove the type name prefix here? We're not using it anywhere else so maybe it's worth removing it here too.

[calebbrown]

I'm keen to ensure the constants are namespaced properly. If they were in a package called dynamicphase then dynamicphase.Install would make sense. Or if they were in a package called dynamic then dynamic.PhaseInstall would make sense.

But outside of that context, I think the prefix helps with readability. For example:

MyFunc(analysisrun.Install)

is less clear than

MyFunc(analysisrun.DynamicPhaseInstall)

There are more characters, but I think the code is more legible.

Go stdlib uses prefixes and suffix in this way. For example:
https://pkg.go.dev/net/http#pkg-constants (Method, Status)
https://pkg.go.dev/[email protected]#pkg-constants (Seek, Err)
https://pkg.go.dev/[email protected]#pkg-variables (IPv4, IPv6)

[maxfisher-g]

Or if they were in a package called dynamic then dynamic.PhaseInstall would make sense.

I like this alternative. Or dynamic.InstallPhase which reads a bit more naturally. I think the same change will eventually need to be made for static analysis 'tasks'

[calebbrown]

After more reflection the problem with dynamic as a package name is that we might have pkg/api/dynamic and internal/dynamic (same for static).

We could decide to never create an internal/dynamic or use an alternatives like dynamicphase, dynamicrun, dynamicexec etc.

[maxfisher-g]

We already have internal/dynamicanalysis and internal/staticanalysis so I don't think we'd need internal/dynamic and internal/static on top of those.

So we could have internal/dynamicanalysis, internal/staticanalysis and pkg/.../dynamic, pkg/.../static.

Or, if you'd prefer the external names to be longer, we can rename the internal ones: internal/dynamic, internal/static and pkg/.../dynamicanalysis, pkg/.../staticanalysis

[calebbrown]

Lol, I've been thinking of dropping analysis from the internal names entirely too, which is why I'm thinking about it.

I'm not convinced yet that DynamicPhase should be separated from analysisrun at this stage. A package with a single tiny data-type seems unnecessary for the time being.

[oliverchang]

This sounds like something we can do in the future as a follow up cleanup?

[maxfisher-g]

Do we actually need an api top-level directory if almost everything is part of the API anyway?

Having an empty api directory (only containing other directories/packages) doesn't seem like a common pattern in other go libraries that we import.

[calebbrown]

Do we actually need an api top-level directory if almost everything is part of the API anyway?

I had considered that, however, all that is in this directory is specifically about interacting with this project and its data.

It is possible that we may build a useful library that is not API related (e.g. the sandbox) that we may want to make public for another project to use. In which case, we would move it into /pkg/.

Changing it isn't super costly either, as only imports need to be updated.

[maxfisher-g]

Nice, I think it's cleaner without FlagUsage.

What's the motivation for introducing a new type for []Ecosystem? Especially for the external API, it seems like we could achieve the same functionality without an extra type by using the existing pattern for analysis.Mode and staticanalysis.Task

[calebbrown]

Nice, I think it's cleaner without FlagUsage.

What's the motivation for introducing a new type for []Ecosystem? Especially for the external API, it seems like we could achieve the same functionality without an extra type by using the existing pattern for analysis.Mode and staticanalysis.Task

I use a type for []Ecosystem to make it easier to format the list for a string.

I can't easily pass []Ecosystem to strings.Join(...) so I manually need to convert each Ecosystem to a string.

To make this process simple I wrap this in a function.

Now the function could have been a regular function (e.g. Join([]Ecosystem) string or AsStrings([]Ecosystem) []string), but its just as easy to create a new type and have it implement the fmt.Stringer interface.

[maxfisher-g]

I don't think we need to introduce a new API type just for joining a slice into a usage string. As you mentioned, this can be a regular function and kept internal to the CLI programs where the string joining in a particular format is needed.

For now I think the code below (comments removed) would be minimally sufficient as an API surface. I'd also say that the None value could be removed, following the precedent here

var SupportedEcosystems = []Ecosystem{
	CratesIO,
	NPM,
	Packagist,
	PyPI,
	RubyGems,
}

func (e *Ecosystem) UnmarshalText(text []byte) error {
	for _, s := range SupportedEcosystems {
		if string(s) == string(text) {
			*e = s
			return nil
		}
	}
	return fmt.Errorf("%w: %s", ErrUnsupported, text)
}

[maxfisher-g]

This code is an example of where I think some effort to shorten the names would be worthwhile.

[calebbrown]

I don't think we need to introduce a new API type just for joining a slice into a usage string. As you mentioned, this can be a regular function and kept internal to the CLI programs where the string joining in a particular format is needed.

For now I think the code below (comments removed) would be minimally sufficient as an API surface. I'd also say that the None value could be removed, following the precedent here

I think redefining types in Go is fairly low-cost.

I'm also not convinced the approach taken to display documentation for analysis.Mode is particularly good either:

• using println is dangerous as the function is intended for language developers and may eventually be removed (https://go.dev/ref/spec#Bootstrapping) - but without it you likely need a custom function to turn the type into a set of strings anyway (due to the way interfaces work)
• analyis.Mode only has two options, so a dedicated option to list two modes doesn't seem necessary.

Finally, encoding.TextMarshaler and encoding.TextUnmarshaler implementations are both need to make the type work, and without None I would still need to write pkgecosystem.Ecosystem("")

So apart from removing the alias type EcosystemSet []Ecosystem and replacing it with a single function func ToStringSlice([]string) I'm not sure there is much worth changing.

[oliverchang]

At the risk of getting into bikeshed territory, I do want to echo Max's comments a little on the Ecosystem/EcosystemSet bits.

This is very clever, but it seems to me there is a bit of a cost to this in terms of code comprehension with these new types and methods getting introduced to do seemingly basic things (e.g. searching a slice, joining a slice), including all the tests needed for those. I'm also not sure how idiomatic this all is -- is this pattern common in Go? A new reader of this code may have a lot of questions trying to unpack what this pkgecosystem code is intending to achieve.

The bigger problem may be consistency -- if we have analysis.Mode doing things in one way, we should be consistent with other similar types like Ecosystem in the same codebase.

[maxfisher-g]

@calebbrown fair point about println, I created #661 to remove it from the project.

[calebbrown]

@calebbrown fair point about println, I created #661 to remove it from the project.

Thanks!

[calebbrown]

PTAL. I've removed the EcosystemSet type and created #663 to cover normalizing enums.

[maxfisher-g]

Nice change! Seems good to me