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 crystal #2

Merged
merged 6 commits into from Mar 13, 2018
Merged

Add crystal #2

merged 6 commits into from Mar 13, 2018

Conversation

@Goryudyuma
Copy link
Collaborator

@Goryudyuma Goryudyuma commented Mar 9, 2018

Crystal-langで書いてみました!

https://crystal-lang.org/

ベンチマークは通りましたが、言語差により細かい挙動はずれているかも知れません。

@Goryudyuma
Copy link
Collaborator Author

@Goryudyuma Goryudyuma commented Mar 9, 2018

実装はGolangをみながら、テンプレートはrubyのやつから改変しているので、少しいびつなところがあるかも知れません。
Golangの実装をそのままCrystalに直したので、Crystalの流儀からは外れているところがあるかも知れません。


Kemal::Session.config do |config|
config.cookie_name = "showwin_happy"
config.secret = "mysession"

This comment has been minimized.

@showwin

showwin Mar 13, 2018
Owner

Goの実装が間違っていますが…

config.cookie_name = "mysession"
config.secret = ENV["ISHOCON1_SESSION_SECRET"] || "showwin_happy"

のつもりでした…!
参考: Ruby実装

comment = env.params.body["content"]? || ""
c_user.create_comment(pid, comment)

env.redirect "/users/"+c_user.id.to_s

This comment has been minimized.

@showwin

showwin Mar 13, 2018
Owner

L122 とココは + の前後にスペース空けた方がよさそうですかね?


Kemal::Session.config do |config|
config.cookie_name = "mysession"
config.secret = "showwin_happy"

This comment has been minimized.

@showwin

showwin Mar 13, 2018
Owner

コメント に書いたように環境変数を優先して頂けると 🙏

config.secret = ENV["ISHOCON1_SESSION_SECRET"] || "showwin_happy"
@showwin
Copy link
Owner

@showwin showwin commented Mar 13, 2018

参考実装の言語追加ありがとうございます👍

動作確認はしていませんが、ざっとコード見て気になった部分だけコメントしました。(細かいところばかりですみません)
あとから問題に気づいたらまたPR送ってください😄
(修正確認次第マージします)

@Goryudyuma
Copy link
Collaborator Author

@Goryudyuma Goryudyuma commented Mar 13, 2018

@showwin 直しました!

@showwin showwin merged commit a511dcf into showwin:master Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants