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

🔥 最大輝度を指定するように修正 #4

Merged
merged 4 commits into from
Jan 31, 2021

Conversation

momeemt
Copy link
Contributor

@momeemt momeemt commented Jan 28, 2021

正しい仕様だったら申し訳ないです。
maxは、そのデータの最大要素ではなく、データで取りうる最大輝度だと思われます。

@jiro4989
Copy link
Owner

jiro4989 commented Jan 28, 2021

上限をそのデータ内の最大値にしていたのはPNMフォーマット的な仕様の成約でそうしたのではなく、このライブラリを実装した時点で僕がそういう振る舞いを必要としていたからです。よって上限をライブラリユーザが任意に制御できるようにすることに反対はありません。

それはそれとして、いきなり引数の数を変更すると後方互換性が壊れるので、すでにライブラリを使ってるユーザに影響がでます。
引数を増減する修正をいれる場合は、引数違いのプロシージャをオーバーロードするか、名前付き引数としてデフォルト値を設定するようにしてください。

@jiro4989 jiro4989 added the feature New feature or request label Jan 28, 2021
@jiro4989
Copy link
Owner

自動テストが動いてなかったので自動テスト修正しました。
masterブランチからrebaseしてもらえますか。

@momeemt
Copy link
Contributor Author

momeemt commented Jan 28, 2021

上限をそのデータ内の最大値にしていたのはPNMフォーマット的な仕様の成約でそうしたのではなく、このライブラリを実装した時点で僕がそういう振る舞いを必要としていたからです。

なるほど、そういうことだったんですね。

最大輝度を引数として取るプロシージャをオーバーロードしました。

proc newPGM*(fileDescriptor: string, col, row: int, data: seq[uint8]): PGM
proc newPPM*(fileDescriptor: string, col, row: int, data: seq[uint8]): PPM

では従来通りにデータの最大値を取るようにしました。

@jiro4989
Copy link
Owner

jiro4989 commented Jan 29, 2021

プロシージャを追加するのであれば、追加するプロシージャにたいしてテストコードを書いてください

https://github.com/jiro4989/pnm/blob/master/tests/tpnm.nim

@momeemt
Copy link
Contributor Author

momeemt commented Jan 29, 2021

newPGM、newPPMに対して輝度を与えて作成したPGM/PPMのテストを追加しました。

@jiro4989
Copy link
Owner

LGTM 👍

LGTM

@jiro4989 jiro4989 merged commit 43c5758 into jiro4989:master Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants