active-hash / active_hash

A readonly ActiveRecord-esque base class that lets you use a hash, a Yaml file or a custom file as the datasource
MIT License
1.2k stars 179 forks source link

Correct fields with YAML aliases in array style #226

Closed stomk closed 3 years ago

stomk commented 3 years ago

When you use aliases in array style YAML, keys used in the aliases are unexpectedly added to the fields.

With the array_products.yml in fixtures:

- /aliases:
  soda_flavor: &soda_flavor
    sweet
  soda_price: &soda_price
    1.0
  chip_flavor: &chip_flavor
    salty
  chip_price: &chip_price
    1.5

- id: 1
  name: Coke
  flavor: *soda_flavor
  price: *soda_price

- id: 2
  name: Pepsi
  flavor: *soda_flavor
  price: *soda_price

- id: 3
  name: Pringles
  flavor: *chip_flavor
  price: *chip_price

- id: 4
  name: ETA
  flavor: *chip_flavor
  price: *chip_price

Before

ArrayProduct.all;
ArrayProduct.field_names
=> [:"/aliases", :chip_flavor, :chip_price, :flavor, :name, :price, :soda_flavor, :soda_price]
ArrayProduct.instance_methods(false).sort
=> [:"/aliases",
 :"/aliases=",
 :"/aliases?",
 :chip_flavor,
 :chip_flavor=,
 :chip_flavor?,
 :chip_price,
 :chip_price=,
 :chip_price?,
 :flavor,
 :flavor=,
 :flavor?,
 :name,
 :name=,
 :name?,
 :price,
 :price=,
 :price?,
 :soda_flavor,
 :soda_flavor=,
 :soda_flavor?,
 :soda_price,
 :soda_price=,
 :soda_price?]

After

ArrayProduct.all;
ArrayProduct.field_names
=> [:flavor, :name, :price]
ArrayProduct.instance_methods(false).sort
=> [:flavor, :flavor=, :flavor?, :name, :name=, :name?, :price, :price=, :price?]
stomk commented 3 years ago

@kbrock Will you please review this?

stomk commented 3 years ago

Do you think this will introduce anything different to a hash of hashes? (without the symbol references)

My last commit introduces a slight change to hash of hashes, where an entry which is not prefixed with / but has /-prefixed fields in it is rejected before, but NOT after. I think this is fine as it is not an expected use case (doc).

coke:
  id: 1
  name: Coke
  flavor: *soda_flavor
  price: *soda_price
  /foo: 1

Before:

shingo:~/lib/tmp/active_hash (master *=) $ git diff
diff --git a/spec/fixtures/key_products.yml b/spec/fixtures/key_products.yml
index 717226f..16a7629 100644
--- a/spec/fixtures/key_products.yml
+++ b/spec/fixtures/key_products.yml
@@ -14,6 +14,7 @@ coke:
   name: Coke
   flavor: *soda_flavor
   price: *soda_price
+  /foo: 1

 pepsi:
   id: 2
shingo:~/lib/tmp/active_hash (master *=) $ bundle exec rspec spec/active_yaml/aliases_spec.rb:48
Run options: include {:locations=>{"./spec/active_yaml/aliases_spec.rb"=>[48]}}
F

Failures:

  1) ActiveYaml::Aliases with YAML hashes .all is expected to eq 4
     Failure/Error: specify { expect(model.all.length).to eq 4 }

       expected: 4
            got: 3

       (compared using ==)
     # ./spec/active_yaml/aliases_spec.rb:48:in `block (4 levels) in <top (required)>'

Deprecation Warnings:

Requiring `rspec/autorun` when running RSpec via the `rspec` command is deprecated. Called from /Users/shingo/lib/tmp/active_hash/spec/spec_helper.rb:4:in `require'.

If you need more of the backtrace for any of these deprecations to
identify where to make the necessary changes, you can configure
`config.raise_errors_for_deprecations!`, and it will turn the
deprecation warnings into errors, giving you the full backtrace.

1 deprecation warning total

Finished in 0.01139 seconds (files took 0.51232 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/active_yaml/aliases_spec.rb:48 # ActiveYaml::Aliases with YAML hashes .all is expected to eq 4

shingo:~/lib/tmp/active_hash (master *=) $ 

After:

shingo:~/lib/tmp/active_hash (stomk/yaml-alias-fields=) $ git diff
diff --git a/spec/fixtures/key_products.yml b/spec/fixtures/key_products.yml
index 717226f..16a7629 100644
--- a/spec/fixtures/key_products.yml
+++ b/spec/fixtures/key_products.yml
@@ -14,6 +14,7 @@ coke:
   name: Coke
   flavor: *soda_flavor
   price: *soda_price
+  /foo: 1

 pepsi:
   id: 2
shingo:~/lib/tmp/active_hash (stomk/yaml-alias-fields *=) $ bundle exec rspec spec/active_yaml/aliases_spec.rb:53
Run options: include {:locations=>{"./spec/active_yaml/aliases_spec.rb"=>[53]}}
.

Deprecation Warnings:

Requiring `rspec/autorun` when running RSpec via the `rspec` command is deprecated. Called from /Users/shingo/lib/tmp/active_hash/spec/spec_helper.rb:4:in `require'.

If you need more of the backtrace for any of these deprecations to
identify where to make the necessary changes, you can configure
`config.raise_errors_for_deprecations!`, and it will turn the
deprecation warnings into errors, giving you the full backtrace.

1 deprecation warning total

Finished in 0.00318 seconds (files took 0.46956 seconds to load)
1 example, 0 failures

shingo:~/lib/tmp/active_hash (stomk/yaml-alias-fields *=) $ 

I believe no other difference to hash of hashes.

stomk commented 3 years ago

Thank you for your review :)

stomk commented 3 years ago

@kbrock Could you release a new version including this change?