ossf/package-analysis

Refactor pkg/

calebbrown opened this issue · 9 comments

calebbrown commented

There are a few issues with the pkg/ directory:

  • api, result are not great package names, as it is easy for it to collide/shadow with other names (see https://google.github.io/styleguide/go/decisions#naming)
  • internal parts should not be in the externally accessible pkgs. internal should depend on pkg not the other way around
  • Much of the pkg dir is "api" related. This should be better reflected in the naming.

Combining the above problems we could restructure the pkg directory into the following:

  • pkg/
    • api/ (this package must remain empty)
      • notification
      • analysisrun (includes RunKey (replaces PkgIdentifier), Result, DynamicRunPhase, Status and strace WriteInfo)
      • ecosystem (includes Ecosystem type)
maxfisher-g commented

Sounds good generally!

Regarding specific naming I think we could tweak analysisrun.RunKey and analysisrun.DynamicRunPhase slightly to make them shorter. Could also just name the package analysis instead of analysisrun.

Some suggestions:

  • (if we use analysis instead of analysisrun) analysis.RunId, analysis.Id; analysis.DynamicRunPhase, analysis.DynamicPhase
  • (if not) analysisrun.Key, analysisrun.Id; analysisrun.DynamicPhase.

I'd opt for the shortest names: analysis, analysis.Id, analysis.DynamicPhase

calebbrown commented

I think analysisrun is better than analysis as there is a lot of scope for conflict, and analysisrun is more specific than just analysis.

So the suggestions to simplify the structs make sense (i.e. Key, DynamicPhase)

maxfisher-g commented

Also, since we have a lot of 'Result' types, perhaps we could make a separate package (analysisresult) to hold all of these?
This may avoid having to have a Result suffix for all of those type names in result.go, especially the prospective analysisrun.DynamicAnalysisResults and analysisrun.StaticAnalysisResults which would be a bit of a mouthful

calebbrown commented

I think cleaning up "Result" is for a subsequent issue.

calebbrown commented

Also, I've been trying to implement this and ecosystem is not a great package name due to shadowing.

I'm thinking of using ossecosystem instead of ecosystem and changing EcosystemNPM to NPM. i.e: api.EcosystemNPM -> ossecosystem.NPM.

(any other suggestions are welcome here)

maxfisher-g commented

Thinking back to when we did #610 ... one alternative I had come up with was using pkgecosystem as the ecosystem package under pkg and renaming internal/pkgecosystem to something else like pkgmanager pkgmanagers or pkgmanagement.

calebbrown commented

I like the idea of using pkg/api/pkgecosystem.

Renaming internal/pkgecosystem make sense. Long term, it's really a library of clients for interacting with package repositories.

What about repoclient?

For terminology sake, I consider "ecosystem" the combination of a language, hosting, and other tooling. And a "repository" the online hosting part of that ecosystem.

maxfisher-g commented

Yeah using repo sounds good. client may be overstating the functionality though, we could use repoaccess? But happy to go with repoclient if you like.

calebbrown commented

Actually I might go with pkgmanager from your earlier suggestion. I think that is probably the best name for now

  • 👍1