Skip to content
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

refactor(lib): Remove environment variable dependencies from lib.List() #118

Merged
merged 3 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,19 @@ var RootCmd = &cobra.Command{
Use: "github-nippou",
Short: "Print today's your GitHub activity for issues and pull requests",
Run: func(cmd *cobra.Command, args []string) {
if err := lib.List(sinceDate, untilDate, debug); err != nil {
list, err := lib.NewListFromCLI(sinceDate, untilDate, debug)
if err != nil {
fmt.Println(err)
os.Exit(1)
}

lines, err := list.Collect()
if err != nil {
fmt.Println(err)
os.Exit(1)
}

fmt.Println(lines)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

linesのstdout出力は元々 lib.List() 内で行われていましたが、ライブラリとして利用する際はstdoutではなくstringを受け取って操作したいです。
そのため呼び出し側でstdout出力するようリファクタリングしました。

},
}

Expand Down
74 changes: 52 additions & 22 deletions lib/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,52 +2,84 @@ package lib

import (
"context"
"fmt"
"log"
"sync"
"time"

"github.com/google/go-github/github"
)

// List outputs formated GitHub events to stdout
func List(sinceDate, untilDate string, debug bool) error {
// List is a struct for collecting GitHub activities.
type List struct {
sinceDate string
untilDate string
user string
accessToken string
settingsGistID string
debug bool
}

// NewList returns a new List.
func NewList(sinceDate, untilDate, user, accessToken, settingsGistID string, debug bool) *List {
return &List{
sinceDate: sinceDate,
untilDate: untilDate,
user: user,
accessToken: accessToken,
settingsGistID: settingsGistID,
debug: debug,
}
}

// NewListFromCLI returns a new List from environment variables or git config.
func NewListFromCLI(sinceDate, untilDate string, debug bool) (*List, error) {
user, err := getUser()
if err != nil {
return err
return nil, err
}

accessToken, err := getAccessToken()
if err != nil {
return err
return nil, err
}
settingsGistID := getGistID()

return &List{
sinceDate: sinceDate,
untilDate: untilDate,
user: user,
accessToken: accessToken,
settingsGistID: settingsGistID,
debug: debug,
}, nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

既存のCLIとしてのユースケースをサポートする NewListFromCLI() を用意しました。 今までと同様に環境変数やgit configから認証情報等を取得しListを返します。

Copy link
Owner

Choose a reason for hiding this comment

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

なるほど。素朴に cmd/root.go にベタ書きすることを考えてましたが、ビジネスロジック(大げさ)は lib/ にあったほうが良さそうですね。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getAccessToken()などはプライベートメソッドだったので、パッケージ外に公開するのはな......と考えこの形にしました 😄
ただ、逆にCLIからしか使われないロジックはcmdパッケージに寄せるのもありかも?とも思ったり 🤔 もしくはcli的なパッケージを切るのもありかもです(やりすぎかもですが)

Copy link
Owner

Choose a reason for hiding this comment

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

あ、そうか。異なるパッケージからは呼べませんでしたね 😅

話逸れますが、C 言語の static 関数はそのファイル内からしか呼べなかったので、ファイル単位でパッケージ化できて、個人的には好きでした。

Go だとディレクトリを分けないといけないのがなあ...。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

おおー、そうなんですね、よさそうですね!RubyやJSだと逆に一度公開しちゃうとどこからでも呼べちゃうのがな...と思ってました 😅


sinceTime, err := getSinceTime(sinceDate)
// Collect collects GitHub activities.
func (l *List) Collect() (string, error) {
sinceTime, err := getSinceTime(l.sinceDate)
if err != nil {
log.Fatal(err)
return "", err
Comment on lines -27 to +59
Copy link
Contributor Author

Choose a reason for hiding this comment

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

log.Fatal() は 内部的に os.Exit(1) してしまうため、ライブラリとしてのユースケースでは困ってしまいます。
そのため単純にerrを返すよう修正しました。

}

untilTime, err := getUntilTime(untilDate)
untilTime, err := getUntilTime(l.untilDate)
if err != nil {
log.Fatal(err)
return "", err
}

ctx := context.Background()
client := getClient(ctx, accessToken)
client := getClient(ctx, l.accessToken)

events, err := NewEvents(ctx, client, user, sinceTime, untilTime, debug).Collect()
events, err := NewEvents(ctx, client, l.user, sinceTime, untilTime, l.debug).Collect()
if err != nil {
return err
return "", err
}
var settings Settings
if err = settings.Init(getGistID(), accessToken); err != nil {
return err
if err = settings.Init(l.settingsGistID, l.accessToken); err != nil {
return "", err
}
format := NewFormat(ctx, client, settings, debug)
format := NewFormat(ctx, client, settings, l.debug)

parallelNum, err := getParallelNum()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: 将来的にはここや、タイムゾーンなども外部から注入できるようにしたいですが、必要に応じてPRを出すつもりです。

if err != nil {
return err
return "", err
}

sem := make(chan int, parallelNum)
Expand All @@ -72,12 +104,10 @@ func List(sinceDate, untilDate string, debug bool) error {

allLines, err := format.All(lines)
if err != nil {
return err
return "", err
}

fmt.Print(allLines)

return nil
return allLines, nil
}

func getSinceTime(sinceDate string) (time.Time, error) {
Expand Down