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

add journals #1

Merged
merged 8 commits into from
Oct 3, 2024
Merged

add journals #1

merged 8 commits into from
Oct 3, 2024

Conversation

yukinagae
Copy link

@yukinagae yukinagae requested a review from kaito2 March 17, 2022 06:48
@yukinagae yukinagae self-assigned this Mar 17, 2022
client.go Outdated
@@ -122,6 +122,85 @@ func (c *Client) postFiles(ctx context.Context,
return c.do(ctx, oauth2Token, req, res)
}

func (c *Client) downloadFile(ctx context.Context,
Copy link
Author

Choose a reason for hiding this comment

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

[comment] client.do() の実装をコピーしているのであまり良くない(※単に client.do() 内でjson.Decodeしている処理を避けてresponse.Bodyのバイトを取得しているだけ)

queryParams url.Values,
) ([]byte, *oauth2.Token, error) {
// TODO: content-typeはcsv以外もあり得る?
req, err := c.newRequest(ctx, apiPath, method, "text/csv", queryParams, nil)
Copy link
Author

Choose a reason for hiding this comment

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

[comment] content-typeを text/csv 固定にしてしまっているが、実際にはPDFなどある

Copy link

@heartscry4423 heartscry4423 Oct 2, 2024

Choose a reason for hiding this comment

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

application/pdfimage/* などをダウンロードできるAPIもあるが、
対応したいのはCSVダウンロードのみなので、命名を具体化してTODOを削除。
・downloadFile() -> downloadCSV()

client.go Outdated
return nil, oauth2Token, res
}
// TODO: bytesではなくBodyをそのまま返した方がいいかも
bytes, err := io.ReadAll(response.Body)
Copy link
Author

Choose a reason for hiding this comment

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

[comment] []byteではなくresponse.Bodyを返した方がよいかも

// 受け付けID
ID int32 `json:"id"`
// TODO: 受け付けメッセージ
Messages *[]string `json:"messages,omitempty"`
Copy link
Author

Choose a reason for hiding this comment

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

[comment] どういう値が入ってくるか?

Copy link

@heartscry4423 heartscry4423 Oct 2, 2024

Choose a reason for hiding this comment

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

運用されてからも長く、ここは気にする必要がないとしてTODOを削除した。
(返り値のサンプルでは id でリクエストを受け付けました。

@yukinagae yukinagae changed the title WIP: add journals [do not merge!] add journals Mar 23, 2022
apiPath string, method string,
oauth2Token *oauth2.Token,
queryParams url.Values,
) (io.ReadCloser, *oauth2.Token, error) {
Copy link
Author

Choose a reason for hiding this comment

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

[comment] io.ReadCloserを返すように変更

@heartscry4423 heartscry4423 changed the base branch from feature/invoices to main September 19, 2024 02:25
@heartscry4423 heartscry4423 changed the title [do not merge!] add journals add journals Oct 2, 2024
Copy link

@kaito2 kaito2 left a comment

Choose a reason for hiding this comment

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

LGTM!!!

(100年 Pending にしていたコメントがあった…)

image

@heartscry4423 heartscry4423 merged commit 3d6ab02 into main Oct 3, 2024
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