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
dedupe #143
dedupe #143
Conversation
e5ade86
to
7e655d1
Compare
Based on @cjhawkins 's PR regarding github it seems like we may need to switch around what we ask for here since we need to know if it's a personal repo or organizational, but we can handle that in another PR. |
ee3049f
to
fa2ad3e
Compare
internal/config/global_config.go
Outdated
|
||
var GetCredentialsPath = getCredentialsPath | ||
|
||
type Projects map[string]*Project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for these to be pointers to structs instead of just structs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also can we call this something more specific? Credentials
and ProjectCredentials
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I need to persist the changes I make to the Project, the map[string]Project doesn't get the updated value otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only because Save
is a pointer receiver. It could also be called like config.Save(projects)
instead of projects.Save()
There are a few things in this PR which look like they are structured in an OOP way which is not how things are typically done in Go.
@@ -0,0 +1,103 @@ | |||
package config_test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests!
log.Fatal(err) | ||
} | ||
} | ||
func TestReadOrCreateUserCredentialsFile(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should really be testing the API of the package rather than the internals. By calling a function like ReadOrCreateUserCredentialsFile
it means that function now has to be exported, and I would think the only exported functions would be LoadUserCredentials
and Save
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, should I create another file for tests like this under the config
package? so I can keep them private and still test them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, though the general practice is try to make your tests in the _test package, then test the exported stuff and not the internals so that your test is coupled to the "API" which should be stable and not the internals which could change. And if you get to the point where the "API" is masking a massive amount of code, try to split it up into smaller packages which can be easily tested in isolation.
ddc7c76
to
ee91d85
Compare
43b0bbc
to
2211d53
Compare
#134
#127
#135
Outstanding