Add root argument #32

chrisatanasian · 2017-10-14T18:29:19Z

nesaulov

Please also fix rubocop errors, so we can see if test pass. You can run $ bundle exec rake before push to make sure that everything is okay.

nesaulov · 2017-10-14T19:13:58Z

README.md

@@ -5,7 +5,7 @@
 [![Gem Version](https://badge.fury.io/rb/surrealist.svg)](https://rubygems.org/gems/surrealist)

 ![Surrealist](surrealist-icon.png)
- 
+


Could you please rollback changes (except actual root documentation) here? Those whitespaces are necessary for adequate readability of README.

Ah, my editor trims whitespaces on save. Fixed.

coveralls · 2017-10-14T20:02:04Z

Coverage increased (+0.03%) to 99.576% when pulling e316b77 on chrisatanasian:add_root_argument into 34aa5bb on nesaulov:master.

nesaulov

A few changes here and we should be ready to go!

nesaulov · 2017-10-14T20:30:49Z

lib/surrealist/carrier.rb

      @camelize                 = camelize
      @include_root             = include_root
      @include_namespaces       = include_namespaces
+      @root                     = root


We need to add sanitization of root parameter to #sanitize! method and specs for it (spec/carrier_spec.rb). Right now there are specs only for strings and nil. It should work with symbols as well (Something.surrealize(root: :wrapper)). I suggest that we raise ArgumentError if root is anything except non-empty string or a symbol (Class, Integer and so on).

What about nil? Seems like the way the code path is setup, Surrealist#call is always expecting something for all of its arguments. In the case of include_root, for example, this is fine because it just defaults to false, which does nothing. But if only non-empty string and a symbol are allowed for root, then there is no way to prevent the schema from being wrapped into root.

Yeah, good point. There are two ways here:

You can default root argument to '' instead of nil and then validate it on not being anything except string or symbol.

You can leave default root argument as it is (nil), and then validate it on being either nil, non-empty string or a symbol.

I think I prefer the second option, it feels more naturally to have nil if we don't want to do anything. What do you think?

I think '2' is better and less arbitrary than empty string. I think the validation/sanitation needs to considers blank strings/symbols. We need to disallow things like " " of :" ", and maybe even consider stripping white space from these args: " hello " => "hello".

This may be something that we should also go back and check on for other params potentially @nesaulov ?

I agree, nil seems more natural to me.

@AlessandroMinali yes, you are right, we need to strip string argument. This is not relevant for other arguments, because they are either booleans or integers. @chrisatanasian could you please add a method in Carrier.sanitize! that will strip root argument?
And also add specs for this case:
Animal.surrealize(root: ' wat ') #=> '{ "wat" => {...} }'

nesaulov · 2017-10-14T20:34:31Z

lib/surrealist/copier.rb

@@ -14,19 +14,39 @@ class << self
      def deep_copy(hash:, klass: false, carrier:)
        namespaces_condition = carrier.include_namespaces || carrier.namespaces_nesting_level != DEFAULT_NESTING_LEVEL # rubocop:disable Metrics/LineLength

-        return copy_hash(hash) unless carrier.include_root || namespaces_condition
+        if !klass && (carrier.include_root || namespaces_condition)


Nice decision! 👍

nesaulov · 2017-10-14T20:35:27Z

lib/surrealist/copier.rb

+      #
+      # @return [Hash] a hash with schema wrapped inside the specified root key.
+      def wrap_schema_into_specified_root(schema:, carrier:)
+        root_key = if carrier.camelize


This looks very similar to #wrap_schema_into_root. Maybe we can DRY it out a bit? 🤔

nesaulov · 2017-10-14T20:35:48Z

lib/surrealist/copier.rb

+      # @param [Object] hash object to be copied.
+      # @param [String] klass instance's class name.
+      # @param [Object] carrier instance of Carrier class that carries arguments passed to +surrealize+
+      # @param [Bool] whether to wrap into namespace.


Missing parameter name.

nesaulov · 2017-10-14T20:37:44Z

spec/carriers/params.rb

-  { camelize: true,  include_namespaces: false, include_root: true, namespaces_nesting_level: 435 },
-  { camelize: true,  include_namespaces: false, include_root: true, namespaces_nesting_level: 666 },
+  { camelize: true,  include_namespaces: true, include_root: true,
+    root: '', namespaces_nesting_level: 3 },


Again, I don't think that root: '' should be valid.

nesaulov · 2017-10-14T20:39:41Z

README.md

+``` ruby
+class Cat
+  include Surrealist
+


Please add a whitespaces here and on line 368.

nesaulov · 2017-10-14T20:41:30Z

README.md

@@ -354,6 +354,34 @@ features (like associations, inheritance etc) are supported and covered. Other O
 issues as well, tests are in progress. All optional arguments (`camelize`, `include_root` etc) are also supported.
 Guides on where to use `#surrealize_collection` vs `#surrealize` for all ORMs are coming.

+### Root
+If you want to wrap the resulting JSON into a specified root key, you can pass optional `root` argument
+to `#surrealize` or `#build_schema`. The root key in this case will be taken from the class name of the


The root key in this case will be taken from the class name of the
surrealizable object.

This sentence is irrelevant here (it is for include_root argument)
Also please add link to here from table of contents.

nesaulov · 2017-10-14T20:41:58Z

@AlessandroMinali what do you think?

coveralls · 2017-10-15T00:12:05Z

Coverage increased (+0.01%) to 99.563% when pulling cb3ecd9 on chrisatanasian:add_root_argument into 34aa5bb on nesaulov:master.

…bol. Add support for root as symbol

coveralls · 2017-10-15T16:51:01Z

Coverage increased (+0.02%) to 99.574% when pulling f5986c1 on chrisatanasian:add_root_argument into 04005d5 on nesaulov:master.

AlessandroMinali · 2017-10-15T14:25:40Z

spec/root_spec.rb

+  end
+end
+
+class CatFood


Just to clean up the spec this can just be a Struct?

AlessandroMinali · 2017-10-15T14:35:51Z

lib/surrealist/carrier.rb

      @camelize                 = camelize
      @include_root             = include_root
      @include_namespaces       = include_namespaces
+      @root                     = root


I think '2' is better and less arbitrary than empty string. I think the validation/sanitation needs to considers blank strings/symbols. We need to disallow things like " " of :" ", and maybe even consider stripping white space from these args: " hello " => "hello".

This may be something that we should also go back and check on for other params potentially @nesaulov ?

nesaulov · 2017-10-15T18:38:17Z

spec/root_spec.rb

+
+      it 'include_root' do
+        expect(JSON.parse(instance.surrealize(root: 'guitar', include_root: true)))
+          .to eq('guitar' => { 'guitar' => { 'brand_name' => 'Fender' } })


Actually, I find this really confusing. If I wrote instance.surrealize(include_root: true, root: 'guitar') I would expect only one root, e.g. { 'guitar' => { 'brand_name' => 'Fender' } } in this case. But I don't know if I am right on this. @AlessandroMinali ?

For what makes sense for me is that root param overrides include_root.

instance.surrealize(include_root: true, root: 'car') # => { 'car' => { 'brand_name' => 'Fender' } } instance.surrealize(root: 'car') # => { 'car' => { 'brand_name' => 'Fender' } } instance.surrealize(include_root: true) # => { 'guitar' => { 'brand_name' => 'Fender' } } instance.surrealize(include_root: false, root: 'car') # => { 'car' => { 'brand_name' => 'Fender' } } instance.surrealize(include_root: false) # => { 'brand_name' => 'Fender' } instance.surrealize() # => { 'brand_name' => 'Fender' }

So would root override include_namespaces too?

@chrisatanasian yeah, I guess so.

…aces

…/surrealist into add_root_argument

coveralls · 2017-10-17T02:27:24Z

Coverage increased (+0.03%) to 99.578% when pulling da1054a on chrisatanasian:add_root_argument into 04005d5 on nesaulov:master.

nesaulov · 2017-10-17T12:58:48Z

@chrisatanasian look good! Please adjust README to reflect the changes that we made since PR was created (how root works with include_namespaces and include_root) and I think this is it.

coveralls · 2017-10-17T16:51:46Z

Coverage increased (+0.03%) to 99.578% when pulling 78375c7 on chrisatanasian:add_root_argument into 04005d5 on nesaulov:master.

coveralls · 2017-10-17T16:51:46Z

Coverage increased (+0.03%) to 99.578% when pulling 78375c7 on chrisatanasian:add_root_argument into 04005d5 on nesaulov:master.

nesaulov

@chrisatanasian Great job! Thank you for your contribution! 🎉

chrisatanasian added 6 commits October 13, 2017 14:55

Add root argument: Add specs and basic argument carrying

6a68a03

Add root argument: Refactor Surrealist::Copier#deep_copy

b5c35c8

Add root argument: Add logic and get tests to pass

e539eac

Add root argument: Add more thorough tests

d31054c

Add root argument: Update README and comments

02ccef2

Merge branch 'master' into add_root_argument

189e9ce

chrisatanasian mentioned this pull request Oct 14, 2017

root_key optional argument #24

Closed

Add root argument: Remove unused parameter

cd63060

nesaulov self-requested a review October 14, 2017 19:12

nesaulov requested changes Oct 14, 2017

View reviewed changes

chrisatanasian added 2 commits October 14, 2017 12:52

Add root argument: Undo README whitespace changes

9aee27a

Add root argument: Fix rubocop errors

e316b77

nesaulov requested changes Oct 14, 2017

View reviewed changes

chrisatanasian added 2 commits October 14, 2017 16:52

Add root argument: Clean up README

f96e9b0

Add root argument: Add missing param name and DRY methods

cb3ecd9

chrisatanasian added 2 commits October 15, 2017 09:46

Add root argument: Raise if root is not nil, non-empty string, or sym…

7fe664e

…bol. Add support for root as symbol

Merge branch 'master' into add_root_argument

f5986c1

AlessandroMinali reviewed Oct 15, 2017

View reviewed changes

nesaulov reviewed Oct 15, 2017

View reviewed changes

chrisatanasian added 4 commits October 16, 2017 10:34

Add root argument: Use struct to clean up specs

b481a88

Add root argument: Make root override include_root and include_namesp…

7c61abc

…aces

Add root argument: Strip root of whitespaces

eedb1df

Merge branch 'add_root_argument' of https://github.com/chrisatanasian…

da1054a

…/surrealist into add_root_argument

AlessandroMinali approved these changes Oct 17, 2017

View reviewed changes

Add root argument: Update README.md to match logic

78375c7

nesaulov approved these changes Oct 17, 2017

View reviewed changes

nesaulov merged commit f2a066e into nesaulov:master Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add root argument #32

Add root argument #32

chrisatanasian commented Oct 14, 2017

nesaulov left a comment

nesaulov Oct 14, 2017

chrisatanasian Oct 14, 2017

coveralls commented Oct 14, 2017 •

edited

Loading

nesaulov left a comment

nesaulov Oct 14, 2017

chrisatanasian Oct 15, 2017

nesaulov Oct 15, 2017

AlessandroMinali Oct 15, 2017

chrisatanasian Oct 15, 2017

nesaulov Oct 15, 2017

chrisatanasian Oct 17, 2017

nesaulov Oct 14, 2017

nesaulov Oct 14, 2017

nesaulov Oct 14, 2017

nesaulov Oct 14, 2017

nesaulov Oct 14, 2017

nesaulov Oct 14, 2017 •

edited

Loading

nesaulov commented Oct 14, 2017

coveralls commented Oct 15, 2017 •

edited

Loading

coveralls commented Oct 15, 2017 •

edited

Loading

AlessandroMinali Oct 15, 2017

AlessandroMinali Oct 15, 2017

nesaulov Oct 15, 2017

AlessandroMinali Oct 15, 2017 •

edited

Loading

nesaulov Oct 15, 2017

chrisatanasian Oct 16, 2017

nesaulov Oct 16, 2017

coveralls commented Oct 17, 2017 •

edited

Loading

nesaulov commented Oct 17, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017 •

edited

Loading

nesaulov left a comment

		@@ -5,7 +5,7 @@
		[![Gem Version](https://badge.fury.io/rb/surrealist.svg)](https://rubygems.org/gems/surrealist)

		![Surrealist](surrealist-icon.png)

Add root argument #32

Add root argument #32

Conversation

chrisatanasian commented Oct 14, 2017

nesaulov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 14, 2017 • edited Loading

nesaulov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nesaulov Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

nesaulov commented Oct 14, 2017

coveralls commented Oct 15, 2017 • edited Loading

coveralls commented Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlessandroMinali Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 17, 2017 • edited Loading

nesaulov commented Oct 17, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017 • edited Loading

nesaulov left a comment

Choose a reason for hiding this comment

coveralls commented Oct 14, 2017 •

edited

Loading

nesaulov Oct 14, 2017 •

edited

Loading

coveralls commented Oct 15, 2017 •

edited

Loading

coveralls commented Oct 15, 2017 •

edited

Loading

AlessandroMinali Oct 15, 2017 •

edited

Loading

coveralls commented Oct 17, 2017 •

edited

Loading

coveralls commented Oct 17, 2017 •

edited

Loading