Eliminate package-level global state#34
Conversation
|
Now ready for review! 🎉 Thank you for your patience 🙇
initBlankContext("OWNER/REPO", "master")
http := initFakeHTTP()
jsonFile, _ := os.Open("../test/fixtures/prList.json")
defer jsonFile.Close()
http.StubResponse(200, jsonFile)If approved, feel free to merge away in your timezone. Lots of things have moved around and interfaces changed, so apologies for the inconvenience of resolving conflicts in other branches. Let me know what you think! Thanks @vilmibm for providing early feedback. ❤️ To figure out in followup PRs:
|
vilmibm
left a comment
There was a problem hiding this comment.
I like the spirit and direction of this! It's a foundation I'm happy to be building on.
I'm a little concerned about the http round tripper initialization being kind of confusing but i understand why it's like that and i'm down to try it for a while.
probablycorey
left a comment
There was a problem hiding this comment.
I think I like this! Its hard to tell without getting in there and trying it out, but it looks good. I'll try and implement the basic gh issue commands with it tomorrow.
Yes I can see how that code is a bit daunting. I'm open to exploring how we can simplify it! Basically, the http.RoundTripper interface is Go's take on middleware, and so lends itself well for dependency injection into an http.Client. I chose this path to inject a RoundTripper that returns stubs in tests, but we can also go about this different ways. |
This is a refactoring with the goal of eliminating global state in git, context, commands, and api packages.
Relying on global state was bad design and ultimately leads to code that is hard to test, as evident by brittle setup in test stubs. Going forward, the code tries to adhere to the following principles:
This is WIP while I figure out how to:
apifromcontext;test.RunCommand;context;config_viewer.goand delegate toapiinstead.