Skip to content

[Ruby版][ADD]NUMERIC対応 #23

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jul 20, 2022
Merged

[Ruby版][ADD]NUMERIC対応 #23

merged 14 commits into from
Jul 20, 2022

Conversation

HokutoMorita
Copy link

@HokutoMorita HokutoMorita commented Jul 5, 2022 •
edited
Loading

やったこと

inputプラグインからstring型で渡された値をColumnOptionを使ってNUMERIC型に変換する対応

残タスク

イシュー

@HokutoMorita HokutoMorita self-assigned this Jul 5, 2022
@HokutoMorita HokutoMorita changed the title [ADD]NUMERIC対応 [Ruby版][ADD]NUMERIC対応 Jul 5, 2022
@HokutoMorita HokutoMorita marked this pull request as ready for review July 8, 2022 07:28
@HokutoMorita HokutoMorita requested a review from d-hrs July 8, 2022 07:28
yosuke-oka
yosuke-oka reviewed Jul 11, 2022
View reviewed changes
Gemfile Outdated
Comment on lines 4 to 6
# INFO: v0.9系のembulkを使用している場合は『gem 'embulk', '< 0.10'』と指定してください
# 参考: https://zenn.dev/hiroysato/articles/957b1b4f77d549
# gem 'embulk'
Copy link

Choose a reason for hiding this comment

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

imo: エラーが出るからバージョン指定てきなコメントのほうがわかりやすいかもです。

Copy link
Author

Choose a reason for hiding this comment

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

承知しました！

@HokutoMorita HokutoMorita requested a review from yosuke-oka July 11, 2022 04:31
This was referenced Jul 11, 2022
Merged
Merged
tksfjt1024
tksfjt1024 reviewed Jul 12, 2022
View reviewed changes
.gitignore Outdated
.tags
your-project-000.json
embulk.jar
config.yml
Copy link

Choose a reason for hiding this comment

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

troccoで config.yml としているだけで config.yaml でも hoge.yml でもいいはずなので、git管理から外すのは本質的ではない気がするんですよね

個人の global gitignore でよしなにやるのでいい気がします

Copy link
Author

Choose a reason for hiding this comment

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

承知です！
それでは、こちら削除します！

Gemfile Outdated

gemspec
gem 'embulk'
gem 'embulk', '< 0.10' # INFO: v0.9系のembulkコマンドの場合エラーになるのでバージョン指定してください。参考: https://zenn.dev/hiroysato/articles/957b1b4f77d549
Copy link

Choose a reason for hiding this comment

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

コメントとバージョン指定の仕方が真逆？な気がします

0.9系が使えないようバージョン指定したいですね

Suggested change
gem 'embulk', '< 0.10' # INFO: v0.9系のembulkコマンドの場合エラーになるのでバージョン指定してください。参考: https://zenn.dev/hiroysato/articles/957b1b4f77d549
gem 'embulk', '>= 0.10' # INFO: v0.9系のembulkコマンドの場合エラーになるのでバージョン指定してください。参考: https://zenn.dev/hiroysato/articles/957b1b4f77d549

Copy link
Author

Choose a reason for hiding this comment

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

0.9系のembulkコマンドを使いたい感じです！
troccoのworkerで動いているembulkが0.9.26なので

コメントを

INFO: v0.9系のembulkコマンドを使う場合はエラーになるのでバージョン指定してください。参考: https://zenn.dev/hiroysato/articles/957b1b4f77d549

のように修正しました。

Copy link

Choose a reason for hiding this comment

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

0.9系を使うとエラーになる、という文章に見えるのですが、まだ良くわかってないですw

どのバージョン使うとエラーになる、みたいな書き方をした方がわかりやすいかもです

Copy link
Author

Choose a reason for hiding this comment

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

v0.9系のembulkコマンドでembulk bundleを実行するとエラーになります。
それを回避するために

gem 'embulk', '< 0.10'

と指定する必要がある感じです。

Copy link
Author

Choose a reason for hiding this comment

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

↑ が伝えたいことになります。

Copy link
Author

Choose a reason for hiding this comment

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

修正しました。

Copy link
Author

Choose a reason for hiding this comment

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

edea856

Copy link

@yosuke-oka yosuke-oka Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ようやく理解しました。

0.9以下を使わないとエラーになるってことですね。

v0.9系のembulkを使用するためのバージョン指定

いまのコメントなら書かなくて良い説ありますが（0.9系を使うのは見たらわかる）
書くとしたら 0.10系には非対応とかですか？
あるいは、0.10以上だとエラーが起きるため とか

Copy link
Author

Choose a reason for hiding this comment

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

確かに、コメント書かなくても良いかもですねw
コメント消しときますね！

Copy link
Author

Choose a reason for hiding this comment

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

削除しました！
ae9c662

@HokutoMorita HokutoMorita requested a review from tksfjt1024 July 12, 2022 04:32
@kekekenta kekekenta removed their request for review July 13, 2022 01:15
@kekekenta
Copy link
Member

（レビュアー多かったので、抜けます

yosuke-oka
yosuke-oka requested changes Jul 13, 2022
View reviewed changes
Copy link

@yosuke-oka yosuke-oka left a comment

Choose a reason for hiding this comment

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

fjtさんのおっしゃってる通りコメント修正お願いします

@HokutoMorita HokutoMorita requested a review from yosuke-oka July 13, 2022 03:45
d-hrs
d-hrs suggested changes Jul 14, 2022
View reviewed changes
embulk-output-bigquery.gemspec Outdated
Gem::Specification.new do |spec|
spec.name = "embulk-output-bigquery"
spec.version = "0.6.13"
spec.version = "0.6.14"
Copy link

Choose a reason for hiding this comment

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

bugfixじゃないので、 0.7.0かな

Suggested change
spec.version = "0.6.14"
spec.version = "0.7.0"

Copy link
Author

Choose a reason for hiding this comment

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

承知しました！

@HokutoMorita HokutoMorita requested a review from d-hrs July 14, 2022 04:58
@HokutoMorita
Copy link
Author

@d-hrs
こちらご確認よろしくお願いいたします！

@HokutoMorita HokutoMorita merged commit 61fade9 into master Jul 20, 2022
@mzumi mzumi mentioned this pull request Dec 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yosuke-oka yosuke-oka yosuke-oka approved these changes

+2 more reviewers

@d-hrs d-hrs d-hrs approved these changes

@tksfjt1024 tksfjt1024 tksfjt1024 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@HokutoMorita HokutoMorita

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HokutoMorita @kekekenta @d-hrs @yosuke-oka @tksfjt1024