Skip to content

Eliminate package-level global state#34

Merged
probablycorey merged 6 commits intomasterfrom
no-global
Oct 30, 2019
Merged

Eliminate package-level global state#34
probablycorey merged 6 commits intomasterfrom
no-global

Conversation

@mislav
Copy link
Copy Markdown
Contributor

@mislav mislav commented Oct 25, 2019

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:

  • Pass information via arguments rather than via package state;
  • Accept information via interfaces rather typed structs (reduces coupling between packages);
  • Tests should run in an isolated environment, i.e. never allowed to read user's or system config values.

This is WIP while I figure out how to:

  • Decouple api from context;
  • Mock context in test.RunCommand;
  • Extract config logic from context;
  • Eliminate config_viewer.go and delegate to api instead.

@mislav mislav marked this pull request as ready for review October 29, 2019 20:20
@mislav
Copy link
Copy Markdown
Contributor Author

mislav commented Oct 29, 2019

Now ready for review! 🎉 Thank you for your patience 🙇

  • Context, API client instances now passed around as arguments (instead of being fetched from package-level)
  • API client now configured from the "outside"
  • FakeHTTP provides ability to stub multiple HTTP responses in a test
	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:

  • ability to stub out responses from CLI child processes such as git status

Copy link
Copy Markdown
Contributor

@vilmibm vilmibm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@probablycorey probablycorey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@probablycorey probablycorey merged commit 06a9b86 into master Oct 30, 2019
@probablycorey probablycorey deleted the no-global branch October 30, 2019 00:18
@mislav
Copy link
Copy Markdown
Contributor Author

mislav commented Oct 30, 2019

I'm a little concerned about the http round tripper initialization being kind of confusing

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.

vilmibm pushed a commit that referenced this pull request Jun 29, 2021
Added in a rough test file
Stonre pushed a commit to Stonre/strands-fork that referenced this pull request Jul 21, 2025
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

Successfully merging this pull request may close these issues.

3 participants