Conversation
| } | ||
| } | ||
|
|
||
| func SmartBaseRepoFunc(f *cmdutil.Factory) func() (ghrepo.Interface, error) { |
There was a problem hiding this comment.
Formerly resolvedBaseRepo I moved this to be near the other BaseRepo function.
| } | ||
| } | ||
|
|
||
| func OverrideBaseRepoFunc(f *Factory, override string) func() (ghrepo.Interface, error) { |
There was a problem hiding this comment.
I extracted this function for testing purposes. I wanted to move this into default to live with the other two BaseRepo functions but ended up with a circular import dependency so I left it here for now.
| s.pagerCommand = cmd | ||
| } | ||
|
|
||
| func (s *IOStreams) GetPager() string { |
There was a problem hiding this comment.
Added for testing purposes.
| return s.IsStdinTTY() && s.IsStdoutTTY() | ||
| } | ||
|
|
||
| func (s *IOStreams) GetNeverPrompt() bool { |
There was a problem hiding this comment.
Added for testing purposes.
There was a problem hiding this comment.
Couldn't testing be achieved through CanPrompt?
There was a problem hiding this comment.
Yep, definitely could. My thought was that there is additional logic s.IsStdinTTY() && s.IsStdoutTTY() in CanPrompt and adding this accessor function was a more direct test since I really just wanted to check that SetNeverPrompt(true) had been called in the test.
mislav
left a comment
There was a problem hiding this comment.
This looks good! Thanks for taking the time to write tests for all of this 💯
| return s.IsStdinTTY() && s.IsStdoutTTY() | ||
| } | ||
|
|
||
| func (s *IOStreams) GetNeverPrompt() bool { |
There was a problem hiding this comment.
Couldn't testing be achieved through CanPrompt?
This PR is a small refactor of
factory/defaultfor readability and testability. There are no behavioral changes. I have often looked atfactory/defaultand found it confusing and hard to parse, additionally it was untested. The changes I have made here make things more clear and I added a lot of test coverage specifically around the threeBaseRepofunctions. Lastly, due to this refactor I was able to move the setup of iostreams prompt and iostreams pager out ofmainand into thefactory/defaultwhich allowed that functionality to finally be tested.