add context package by vilmibm · Pull Request #17 · cli/cli

vilmibm · 2019-10-14T16:03:55Z

This was an attempt at moving some stuff out of github and into a new package, context. It also houses some code that @probablycorey added in his graphql PR.

It's entirely possible that this isn't the direction we want to go in and I'm open to that. I wanted to open this PR anyway since afaik it doesn't break anything and includes a fair bit of code trimming.

Punted things:

config code still lives in github. it was too tightly coupled with github.client, which is still in use in establishing auth context. since @mislav is redoing our auth flow i ultimately just decided to leave this alone after many attempts to cleanly disentangle
I wanted to look ahead to more prototype backporting and get rid of localrepo but i didn't get to that
I was going to take out github host configuration since we don't really need it right now and are already hardcoding the graphql api endpoint but this was also tightly coupled with github.client

This reverts commit 7427716.

probablycorey

I want to use this! I had a few questions about naming and errors.

context/ghrepo.go

probablycorey · 2019-10-14T17:27:08Z

api/queries.go

-	repo := project.Name
-	currentBranch := currentBranch()
+	ghRepo, rerr := context.CurrentGitHubRepository()
+	if rerr != nil {


I'm guessing that using rerr over err has some benefit here, but I can't tell what it is.

something that is plaguing me in golang is this scenario:

foo, err := someFunction() if err != nil { return err } bar, err := otherFunction() // fails because err is already defined and i'm trying to use := operator

i've been slapping letters onto my err variable names to make them distinct and allow group assignment / initialization. i assume there is a better way to do this and i'm all ears.

Then I have good news for you! You can reuse the err variable with short variable declarations, as long as you declares multiple variables with :=. This code compiles fine!

foo, err := os.Open("file.moo") if err != nil { panic(err) } bar, err := os.Open("file.go") fmt.Printf("%v %v %v", foo, bar, err)

Unlike regular variable declarations, a short variable declaration may redeclare variables provided they were originally declared earlier in the same block (or the parameter lists if the block is the function body) with the same type, and at least one of the non-blank variables is new. As a consequence, redeclaration can only appear in a multi-variable short declaration. Redeclaration does not introduce a new variable; it just assigns a new value to the original.

via https://golang.org/ref/spec#Short_variable_declarations

OH

i didn't even try because i knew := failed for singular assignment

welp, thank you for telling me! i think this PR no longer has the subtly renamed errs but i'll keep it in mind for the future

api/queries.go

probablycorey · 2019-10-14T17:38:51Z

api/queries.go

+	currentUsername, err := context.CurrentUsername()
+	if err != nil {
+		return PullRequestsPayload{}, err
+	}


To get the branch, repo and username there is a lot of error checking. Would it be simpler to call context.GetContext() that returns a struct with the data and an err?

This would also make it easier to test as well because we could mock what is returned by GetContext()

probablycorey · 2019-10-14T17:41:31Z

context/remote.go

 }
+
+// GuessRemote attempts to select and return the remote a user likely wants to target when dealing with GitHub repositories.
+func GuessRemote() (Remote, error) {


We pretty much to guess which remote to use, but naming the function that makes it feel more random than it is? Would DefaultRemote work?

GuessRemote felt honest; DefaultRemote makes it sound like we have some hardcoded default independent of the repo we're in. While not truly random, we are picking a remote without any actual information from the user that it's what they want.

Yeah, I guess I'm trying to pretend we aren't guessing, which doesn't really communicate what it does. So GuessRemote works, GuessDefaultRemote might be more descriptive, but I'm totally fine with GuessRemote now too.

My hope is a name like GuessRemote will motivate us to have a better system for remote determination ^_^

mislav · 2019-10-17T00:39:21Z

💫 Now ready for review! ✨

// Context represents the interface for querying information about the current environment
type Context interface {
	AuthToken() (string, error)
	AuthLogin() (string, error)
	Branch() (string, error)
	SetBranch(string)
	Remotes() (Remotes, error)
	BaseRepo() (*GitHubRepository, error)
	SetBaseRepo(string)
}

// example usage:
branch, err := context.Current().Branch()

Design goals:

lazy loading, i.e. individual data is only fetched when requested
caching, i.e. data is never re-requested from the filesystem
unit-tested code
command tests run with their own isolated context
ability to override parts of context via --repo, -R and --current-branch, -B global cli flags
setting GH_REPO=foo/bar is equivalent to gh --repo foo/bar
🔥 the legacy github package!

@vilmibm To reach the above goals, I had to restructure and rename a lot of the existing context comands. Your code served as a great outline for me, but in the end I mostly rewrote the whole branch. I hope you don't mind! Please let me know what you think 🙇

Breaking:

If there isn't a well-formed ~/.config/hub file, the program will panic. We will refine authentication and the initialization of a fresh config file with OAuth authentication flow #6.

mislav · 2019-10-17T14:24:38Z

Update:

keep original order of git remotes (applies when using remotes.FindByName('*'))
avoid crash on malformed yaml config file
restructure SSH config parser (used when translating git@... or ssh:// remote URLs), avoid ever reading system or user ~/.ssh/config files in tests
more tests!

mislav

https://github.slack.com/archives/CLLG3RMAR/p1571322620091200?thread_ts=1571273015.090400&cid=CLLG3RMAR

probablycorey

This is wonderful, it's going to make the code so much easier to write and maintain! I have a few questions:

Is there an advantage to using context.Current() over automatically creating currentContext in the package's Init function? I think we could still mock the context in tests by calling context.InitBlankContext and we could call context.Whaterver() instead of context.Current.Whaterver()
The context.BaseRepo was a little confusing to me. It made me think it was contained info about the git repo, and then I was wondering if there are other non-base repos. But if it was called something like GitHubRepo it would have clicked more quickly.
AuthLogin makes me thing this method with log me in rather than return the username. Maybe Username or AuthUsername?

P.S. This is my drug 👇

mislav · 2019-10-17T17:08:45Z

Is there an advantage to using context.Current() over automatically creating currentContext in the package's Init function?

That's a great question. Right now it doesn't seem like there is a clear advantage, but when designing this, I was trying to be a bit defensive about putting all context methods as being "global" in the context.* scope, and instead made the context an instance that you can obtain a reference to (and swap it around during tests). I've done so that commands could theoretically hold on to their own context instance in the future and that could potentially enable running tests in parallel. If we ran them in parallel now, they would experience race conditions in which one test initializing a mock context could affect the run of another test.

But as things are wired up right now, there's not a clear benefit from calling context.Current().Branch() vs. context.Branch(), except perhaps revealing that there is an underlying context instance which is stateful.

BaseRepo / AuthLogin

This is good feedback around naming. I'll try to rename them according to your suggestions so that their intent is more immediately apparent.

fix addArgs and combine by addding branchName parameter

Remove Terminal, no longer needed by ghcs

Add common badges for python projects Co-authored-by: Mackenzie Zastrow <zastrowm@users.noreply.github.com>

nate smith added 11 commits October 10, 2019 14:16

Merge remote-tracking branch 'origin/graphql' into ghr-context

0e6a693

add context.go and move over getToken

31b0163

move more stuff into context.go

8558c27

add GitHubRepository

457ef18

remove some dead code

d888a3b

remove more dead code

331adee

remove project in favor of ghr

175c78c

move remote to context package

cec6d24

remove unused method

3b635d5

towards moving config into context

7427716

Revert "towards moving config into context"

cc31084

This reverts commit 7427716.

vilmibm requested review from mislav and probablycorey October 14, 2019 16:08

probablycorey reviewed Oct 14, 2019

View reviewed changes

nate smith and others added 6 commits October 15, 2019 13:36

switch to context struct

91f4832

Merge remote-tracking branch 'origin/master' into ghr-context

b70d051

Create overridable Context interface

fe88746

Test remote parsing

fb72fe0

🔥 github package

3d4c08f

go mod tidy

a171e5f

mislav force-pushed the ghr-context branch from b3628d5 to a171e5f Compare October 17, 2019 00:26

vilmibm mentioned this pull request Oct 17, 2019

Tasks week of 10/14 #16

Closed

13 tasks

mislav added 7 commits October 17, 2019 13:46

Keep original order of remotes

70d7444

Improve config file parsing

c777bba

Use eq in more tests

048fa00

Test SSH config parser

6724241

Ensure remote URL parsing tests don't read user SSH config files

2a34a7f

Add test for invalid remote URLs

ce6aa87

Test FindByName

c040e7c

mislav approved these changes Oct 17, 2019

View reviewed changes

mislav merged commit cb95603 into master Oct 17, 2019

mislav deleted the ghr-context branch October 17, 2019 15:30

probablycorey reviewed Oct 17, 2019

View reviewed changes

vilmibm pushed a commit that referenced this pull request Jun 29, 2021

Merge pull request #17 from ttran112/trunk

c127ca4

fix addArgs and combine by addding branchName parameter

mislav pushed a commit that referenced this pull request Sep 28, 2021

Merge pull request #17 from github/rm-terminal

2de51a8

Remove Terminal, no longer needed by ghcs

Stonre pushed a commit to Stonre/strands-fork that referenced this pull request Jul 21, 2025

fix: Update readme to include badges (cli#17)

6decbd8

Add common badges for python projects Co-authored-by: Mackenzie Zastrow <zastrowm@users.noreply.github.com>

Conversation

vilmibm commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

probablycorey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mislav commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mislav commented Oct 17, 2019

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

probablycorey left a comment

Choose a reason for hiding this comment

Uh oh!

mislav commented Oct 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vilmibm commented Oct 14, 2019 •

edited

Loading

mislav commented Oct 17, 2019 •

edited

Loading