search

google / addlicense

A program which ensures source code files have copyright license headers by scanning directory patterns recursively
Apache License 2.0
724 stars 170 forks source link

[Proposal] Usage as module instead of CLI #123

Open CalebAlbers opened 2 years ago

CalebAlbers commented 2 years ago

I'm looking to integrate addLicense into an existing suite of internal FOSS compliance-related tooling, and part of that would be eased by decoupling the addLicense functions from the CLI-specific code that uses them. I'd like to propose the following changes (which I will happily create a PR for), but I wanted to check in with maintainers prior to doing so:

  1. Turn main() into a function that consists solely of flag parsing and calling a new Run() function
  2. Break out the rest of main() into a Run() function that takes in the parsed flag values and executes everything else (goroutines, etc)
  3. Export fileMatches, addLicense, licenseHeader, isGenerated, and hasLicense

@willnorris et al, are there any thoughts or concerns about that approach? My over-arching goal is to increase composability.

willnorris commented 2 years ago

pulling logic out of main() would certainly make things easier to test as well, so I'm supportive in theory.

That said, I'd want to be very deliberate about what API surface we're exposing. I've been somewhat fast and loose with some of the refactors I've done in the past because it was all internal anyway. That wouldn't be the case here. One way to guard against that is to keep the exported API as bare minimum as possible. Is it really necessary to export all of those methods? I'm not sure.

/cc @mco-gh directly for opinions

mco-gh commented 2 years ago

I'm not opposed to this proposal but don't have much in the way of cycles these days for this project so I may not have time to review any large PRs but if you're ok with it Will, then I'm fine too.

CalebAlbers commented 2 years ago

That said, I'd want to be very deliberate about what API surface we're exposing.

I'm 100% on board with that. To better inform the discussion, I pushed some potential changes into draft PR #124. I took your advice and stuck to exporting the bare minimum, which at this point is just a Run function. I purposely did some things like leaving the deprecated skipExtensionFlags option in main, passing only a ignorePatterns argument instead.

I also explored some options like passing a *log.Logger interface around instead of calling log functions directly. There's still work to be done as far as figuring out how to handle the fact that checkonly calls OS.exit(1), etc.

It's rough around the edges, but I'd love to use it as a stepping stone for further discussion. Thank you both for being so open to considering a refactor!