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

feat!: JSON and YAML migration (Mapping -> Serializable) #34

Merged
merged 39 commits into from
Mar 8, 2022

Conversation

caspiano
Copy link
Contributor

@caspiano caspiano commented Aug 2, 2021

Fast-forward #28 to align with master.

Resolves #19
Resolves #14
Resolves #25

dukenguyenxyz and others added 30 commits August 2, 2021 10:46
Co-authored-by: Caspian Baska <caspianbaska@gmail.com>
.github/workflows/CI.yml Outdated Show resolved Hide resolved
end
# Returns a Tuple of the previous and the current
# value of an instance variable if it has changed
def {{name}}_change : Tuple({{opts[:klass]}}?, {{opts[:klass]}}?)?
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
def {{name}}_change : Tuple({{opts[:klass]}}?, {{opts[:klass]}}?)?
def {{name}}_change : Tuple({{opts[:klass]}}?, {{opts[:klass]}})?

If there's a change, that should be to the type of the field (I think?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot type this, currently raise the following

error in line 2
Error: while requiring "./spec/model_spec.cr"


In spec/model_spec.cr:410:13

 410 | klass.string_change.should eq ({nil, "hello"})
             ^------------
Error: instantiating 'Inheritance#string_change()'


There was a problem expanding macro 'finished'

Called macro defined in macro 'inherited'

 20 | macro finished

Which expanded to:

 >  1 |       __process_attributes__
 >  2 |       __customize_orm__
 >  3 |       __track_local_fields__
 >  4 |       
 >  5 |       __track_changes__
 >  6 |       __map_json__
 >  7 |       __create_initializer__
 >  8 |       __getters__
 >  9 |       __nilability_validation__
 > 10 |       
 > 11 |     
Error: expanding macro


There was a problem expanding macro '__track_local_fields__'

Called macro defined in src/active-model/model.cr:274:3

 274 | macro __track_local_fields__

Which expanded to:

 >  1 |     
 >  2 |       @[JSON::Field(ignore: true)]
 >  3 |       @[YAML::Field(ignore: true)]
 >  4 |       getter? string_changed  = false
 >  5 | 
 >  6 |       # Include `string` in the set of changed attributes, whether it has changed or not.
 >  7 |       def string_will_change! : Nil
 >  8 |         @string_changed = true
 >  9 |         @string_was = @string.dup
 > 10 |       end
 > 11 | 
 > 12 |       @[JSON::Field(ignore: true)]
 > 13 |       @[YAML::Field(ignore: true)]
 > 14 |       getter string_was : String | Nil = nil
 > 15 | 
 > 16 |       # Returns a Tuple of the previous and the current
 > 17 |       # value of an instance variable if it has changed
 > 18 |       def string_change : Tuple(String?, String)?
 > 19 |         {@string_was, @string} if string_changed?
 > 20 |       end
 > 21 |     
 > 22 |       @[JSON::Field(ignore: true)]
 > 23 |       @[YAML::Field(ignore: true)]
 > 24 |       getter? integer_changed  = false
 > 25 | 
 > 26 |       # Include `integer` in the set of changed attributes, whether it has changed or not.
 > 27 |       def integer_will_change! : Nil
 > 28 |         @integer_changed = true
 > 29 |         @integer_was = @integer.dup
 > 30 |       end
 > 31 | 
 > 32 |       @[JSON::Field(ignore: true)]
 > 33 |       @[YAML::Field(ignore: true)]
 > 34 |       getter integer_was : Int32 | Nil = nil
 > 35 | 
 > 36 |       # Returns a Tuple of the previous and the current
 > 37 |       # value of an instance variable if it has changed
 > 38 |       def integer_change : Tuple(Int32?, Int32)?
 > 39 |         {@integer_was, @integer} if integer_changed?
 > 40 |       end
 > 41 |     
 > 42 |       @[JSON::Field(ignore: true)]
 > 43 |       @[YAML::Field(ignore: true)]
 > 44 |       getter? no_default_changed  = false
 > 45 | 
 > 46 |       # Include `no_default` in the set of changed attributes, whether it has changed or not.
 > 47 |       def no_default_will_change! : Nil
 > 48 |         @no_default_changed = true
 > 49 |         @no_default_was = @no_default.dup
 > 50 |       end
 > 51 | 
 > 52 |       @[JSON::Field(ignore: true)]
 > 53 |       @[YAML::Field(ignore: true)]
 > 54 |       getter no_default_was : String | Nil = nil
 > 55 | 
 > 56 |       # Returns a Tuple of the previous and the current
 > 57 |       # value of an instance variable if it has changed
 > 58 |       def no_default_change : Tuple(String?, String)?
 > 59 |         {@no_default_was, @no_default} if no_default_changed?
 > 60 |       end
 > 61 |     
 > 62 |   
Error: method Inheritance#string_change must return (Tuple(String | Nil, String) | Nil) but it is returning (Tuple(String | Nil, String | Nil) | Nil)

src/active-model/model.cr Outdated Show resolved Hide resolved
self.{{name}} = model.{{name}} if data.has_key?({{name.stringify}}) && self.{{name}} != model.{{name}}
# Assign each field from JSON if field exists in JSON and has changed in model
def assign_attributes_from_trusted_json(json)
json = json.read_string(json.read_remaining) if json.responds_to? :read_remaining && json.responds_to? :read_string
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these #responds_to? calls be replaced with a proper type check instead (possibly as a restriction on the method argument)?

Copy link
Contributor

Choose a reason for hiding this comment

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

cannot type the method argument here, possible types include JSON::PullParser (or YAML::PullParser in the case of YAML), String, IO, however we have to do a runtime check since only IO::Sized < IO responds to read_remaining, which we cannot detect from arg_type and can only do so once json.read_string (i.e. record the string then read_next

Comment on lines +598 to +604
# - `json_key`: Override the emitted JSON key.
# - `json_emit_null`: Emit `null` if value is `nil`, key is omitted if `json_emit_null` is `false`.
# - `json_root`: Assume value is inside a JSON object under the provided key.
# ## YAML
# - `yaml_key`: Override the emitted YAML key.
# - `yaml_emit_null`: Emit `null` if value is `nil`, key is omitted if `yaml_emit_null` is `false`.
# - `yaml_root`: Assume value is inside a YAMLk object under the provided key.
Copy link
Contributor

Choose a reason for hiding this comment

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

As converter applies to both JSON and YAML serialisation, it would be good to either:

  1. provide the ability to specify seperate JSON / YAML converters, or
  2. provide the ability to specify key, emit_null and root that's used in both JSON and YAML (unless otherwise specified).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer these options to default to the unprefixed argument, with the option to override via a prefixed argument

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I'm not exactly sure what this means, could you please clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kim: json_converter and yaml_converter
Me: Support unprefixed arguments, like root, that is passed to both the json and yaml prefixed arguments. If, in my example, json_root is defined, use that instead of root

Co-authored-by: Kim Burgess <kim@place.technology>
@caspiano caspiano changed the title Rebase of #28 feat: JSON and YAML migration (Mapping -> Serializable) Aug 9, 2021
@caspiano caspiano changed the title feat: JSON and YAML migration (Mapping -> Serializable) feat!: JSON and YAML migration (Mapping -> Serializable) Mar 8, 2022
@caspiano caspiano merged commit a76c35b into master Mar 8, 2022
@caspiano caspiano deleted the json-yaml-rebase branch March 8, 2022 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants