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 version column to Timestamp table #200

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

Yamaguchi
Copy link
Contributor

No description provided.

@Yamaguchi Yamaguchi force-pushed the add_version_column_to_timestamp branch from df5e216 to f884b6f Compare January 10, 2024 06:32
@rantan
Copy link
Contributor

rantan commented Jan 10, 2024

Timestampモジュールの最上位あたりに、バージョニングの説明と各バージョンの挙動の差分が説明されたコメントが欲しいです。

@@ -14,6 +14,7 @@ class CreateTimestamp < ActiveRecord::Migration<%= migration_version %>
t.bigint :prev_id
t.boolean :latest, null: false, default: true
t.boolean :hex, null: false, default: false
Copy link
Contributor

Choose a reason for hiding this comment

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

hexフラグは不要 by @azuchi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

67c0415 で削除してます

@@ -53,7 +56,8 @@ def initialize(attributes = nil)
status: :init,
timestamp_type: attributes[:timestamp_type] || :simple,
prev_id: attributes[:prev_id],
hex: hex
hex: hex,
version: attributes[:version] || "1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version: attributes[:version] || "1"
version: attributes[:version] || "2"

はどうでしょう?

Copy link
Contributor

Choose a reason for hiding this comment

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

デフォルト値は不要 by @azuchi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

67c0415 で修正してます

@Yamaguchi Yamaguchi force-pushed the add_version_column_to_timestamp branch 3 times, most recently from 98d6739 to d62180c Compare January 11, 2024 05:36
@Yamaguchi
Copy link
Contributor Author

Timestampモジュールの最上位あたりに、バージョニングの説明と各バージョンの挙動の差分が説明されたコメントが欲しいです。

lib/glueby/contract/timestamp.rb のTimestampクラスに説明を追記してます。

@Yamaguchi Yamaguchi force-pushed the add_version_column_to_timestamp branch from d62180c to 67c0415 Compare January 11, 2024 06:41
lib/glueby/contract/timestamp.rb Show resolved Hide resolved
lib/glueby/contract/timestamp.rb Outdated Show resolved Hide resolved
# - version [String] Version of the timestamp recording method.
# The format in which the timestamp is recorded differs depending on the version.
# Version "1" treats the specified content and prefix as a binary string.
# Version "2" treats the specified content and prefix as a hexadecimal string with the string set to prefix and content.
Copy link
Contributor

Choose a reason for hiding this comment

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

version が 1, 2 以外の場合は ArgumentError にした方が良いと思います。

Copy link
Contributor

Choose a reason for hiding this comment

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

バリデーションでチェックしているようですね。

@Yamaguchi Yamaguchi merged commit 6e23585 into chaintope:master Jan 17, 2024
4 checks passed
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