-
-
Notifications
You must be signed in to change notification settings - Fork 688
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
Messages: ruby: Convert DTOs attributes from camelCase to snake_case #1601
Conversation
|
||
# Thank you very munch rails! | ||
# https://github.com/rails/rails/blob/v6.1.3.2/activesupport/lib/active_support/inflector/methods.rb#L92 | ||
def underscore(camel_cased_word) |
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.
It seems to me that converting to snake_case
complicates everything a lot. If we weren't doing this, we could generate JSON simply by doing JSON.stringify
.
I know snake_case
is a convention in Ruby, but here we're dealing with an external data format, and I think the additional complexity of snake case is difficult to justify.
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.
There is no such thing as JSON.stringify in ruby.
JSON.generate works with hashes only, not with objects. So we will have to write serializers anyway.
snake_case over camelCase was one of the point into generating DTOs over hashes.
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.
I think the message DTOs need to follow Ruby idioms. The whole point of adding the DTOs, for me, is to add a layer of indirection between the raw messages and client code, precisely so that we can do things like this.
Can you same more about the JSON.stringify use case? I didn't realise there were instances where we need to convert these objects back to raw JSON messages.
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.
They need to be converted to JSON / NDJSON for the message formatter, and also for publishing to reports.cucumber.io.
The only users of the Cucumber Messages Ruby API is us (the internal Cucumber-Ruby codebase). Going the roundabout way of using snake case seems completely unnecessary to me.
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.
I think the message DTOs need to follow Ruby idioms. The whole point of adding the DTOs, for me, is to add a layer of indirection between the raw messages and client code, precisely so that we can do things like this.
Can you same more about the JSON.stringify use case? I didn't realise there were instances where we need to convert these objects back to raw JSON messages.
The formatters output messages as ndjson
Also gherkin as a standalone provide a bin which allows to output ndjson from those messages
cf the CI here: #1603
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.
snake_case over camelCase was one of the point into generating DTOs over hashes.
That wasn't obvious to me. For me, the only benefit was that we can do envelope.testStepFinished.timestamp
rather than having to do envelope[:testStepFinished][:timestamp]
.
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.
Another benefit of DTOs is that we can throw an exception in the constructor if you pass an unsupported property.
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 only users of the Cucumber Messages Ruby API is us (the internal Cucumber-Ruby codebase). Going the roundabout way of using snake case seems completely unnecessary to me.
There is at least one project which uses gherkin directly, and that may be impacted with changes in messages. Cf. #1559
Also, formatters may deal with messages directly. Thus, custom formatters may be impacted.
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.
There is no such thing as JSON.stringify in ruby.
My bad. We could do this:
require 'json'
class Timestamp
attr_reader :seconds
attr_reader :nanos
def initialize(
seconds: 0,
nanos: 0
)
@seconds = seconds
@nanos = nanos
end
def to_hash
return {
seconds: seconds,
nanos: nanos
}
end
end
t = Timestamp.new(seconds: 10, nanos: 33)
puts t.to_hash.to_json
The point of the ruby DTOs was to stick with ruby standard.
We missed to convert cameCase attribute names to snake_case in the previous PR: #1574