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
10704 better odata types #745
Conversation
Otherwise there's no easy way to tell
Metadata type could be improved later, but at least the JSON data won't need to be modified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very smooth. Exciting to see us taking more advantage of the OData stuff!
Just a few small things to address.
@@ -7,6 +7,8 @@ class ResponseJsonGenerator | |||
|
|||
attr_accessor :response | |||
|
|||
BASE_URL_SIGNIFIER = "__NEMO_HOST__" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we call these placeholders rather than signifiers in the codebase I think.
|
||
def location_value(answer) | ||
Answer::LOCATION_COLS.map do |key| | ||
[key.titleize, answer.attributes[key]&.to_f] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is to_f needed here? I thought they were already floats in the DB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, they are returned as BigDecimal which does unusual things during conversions. For example:
# Scientific notation
[123, answer.attributes[key]].to_s
=> [123, 0.15937378e2]
# Stringified for some reason
[123, answer.attributes[key]].to_json
=> [123,"15.937378"]
video_path = Rails.root.join("spec", "fixtures", "media", "video", "jupiter.mp4").to_s | ||
image_path = Rails.root.join("spec/fixtures/media/images/the_swing.png") | ||
audio_path = Rails.root.join("spec/fixtures/media/audio/powerup.mp3") | ||
video_path = Rails.root.join("spec/fixtures/media/video/jupiter.mp4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why this got changed back. See Rails::FilePath cop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default must have changed in a newer version of Rubocop (it was recently upgraded for NEMO). We don't explicitly specify anything in our config.
I can't immediately find the changelog, but see e.g. rubocop/rubocop-rails#195 for reasoning. Are you good with the new default?
"DatetimeQ11": "2015-10-12T12:15:12-06:00", | ||
"DateQ12": "2014-11-09", | ||
"TimeQ13": "23:15:00", | ||
"ImageQ14": null | ||
"ImageQ14": "__NEMO_HOST__/en/m/mission1/media/images/*image_id1*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤩
Thanks Tom! Fixed "placeholder" naming, agree that's much better. Will merge after CI passes. |
Changes