-
Notifications
You must be signed in to change notification settings - Fork 10
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 Field
class
#11
Add Field
class
#11
Conversation
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!
I added a few comments. And also one thing I think is missing - connection between Schema and Field in methods like schema.fields
, schema.get_fields
etc.
@@ -209,14 +207,31 @@ rows = [ | |||
['wrong column count'] | |||
] | |||
|
|||
schema.convert(rows) | |||
schema.cast(rows) |
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.
From my experience almost all interactions with schema related to casting go thru schema.cast_row
- not sure this cast method needed at all. Also name could be confusing (schema.cast
- cast what? schema? data?). WDYT?
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.
Ah, this was carried over from the old Python API - calling it cast_rows
would probably make more sense
@@ -28,6 +28,7 @@ | |||
require "jsontableschema/types/string" |
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 suppose it's something like __init__.py
in Python? I'm not sure how it works in Ruby but in Python we use this to declare API - adding only public interface and not adding internal stuff (like types after field introduction). Like it's internal API could be changed anytime without any notification etc.
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.
Yeah, that's it, this is the main file that requires everything needed by the gem. There are other patterns (i.e. we could modularise, so you could only require jsonschema/table
or jsonschema/schema
), but this is the generally accepted pattern, especially for small, focussed gems.
@roll The schema and field connections are defined in https://github.com/theodi/jsontableschema.rb/blob/feature-add-field/lib/jsontableschema/model.rb - which is a Ruby module which is included in schema.rb - I've split a lot of things out into modules, which keeps things neater. |
Fixes #9
@roll, @pwalsh - Does this chime with your expectations in #9?