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

feat: add term and balance. #52

Merged
merged 12 commits into from
Apr 9, 2024
Merged

feat: add term and balance. #52

merged 12 commits into from
Apr 9, 2024

Conversation

nonz250
Copy link
Contributor

@nonz250 nonz250 commented Mar 29, 2024

Overview

Changes due to the addition of the Term and Balance objects.

Changes

@nonz250 nonz250 changed the title Nonz250/add term balance feat: add term and balance. Mar 29, 2024
@nonz250 nonz250 requested a review from niheeeee March 29, 2024 09:54
src/index.ts Outdated Show resolved Hide resolved
src/term.ts Outdated Show resolved Hide resolved
Copy link

@niheeeee niheeeee left a comment

Choose a reason for hiding this comment

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

リリースバージョンが更新されてないと思うので確認お願いします。
あとlistについて1点確認コメントしました。

src/index.ts Outdated
type?: "sales" | "service_fee" | "transfer_fee",
}

export interface TermListOptions extends ListOptions {
Copy link

Choose a reason for hiding this comment

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

[ask]
ListOptionsを継承した場合に、Termオブジェクトだとcreatedが無いので since/until が対象無いのは明確にはしない方針でしたっけ?
動作は特に問題ないはず。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pagination Option を追加してみました。
以前口頭で話したと思いますが、この作りは治安を悪くしそうで少し不安です。
現状すぐに対応できる案として、ネーミングで治安維持を図ろうと思いますが、他にいい案あれば教えてほしいです。

Copy link

Choose a reason for hiding this comment

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

Pagination と CreatedRange に分離するというのは、既に他でもある概念なので問題ないと思います。
今後さらに追加や分離となりそうな場合には考え直しが必要そう。

@nonz250
Copy link
Contributor Author

nonz250 commented Apr 9, 2024

リリースバージョンが更新されてないと思うので確認お願いします。 あとlistについて1点確認コメントしました。

version 上げました。

@nonz250 nonz250 merged commit 25a64e2 into master Apr 9, 2024
4 checks passed
@nonz250 nonz250 deleted the nonz250/add-term-balance branch April 9, 2024 07:28
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.

2 participants