Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Usage as module instead of CLI #123

Open
CalebAlbers opened this issue Aug 10, 2022 · 3 comments
Open

[Proposal] Usage as module instead of CLI #123

CalebAlbers opened this issue Aug 10, 2022 · 3 comments

Comments

@CalebAlbers
Copy link

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
Copy link
Collaborator

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
Copy link
Member

mco-gh commented Aug 10, 2022

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
Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants