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

merged order wrong? #6

Closed
NicoLugil opened this issue Nov 13, 2019 · 10 comments · Fixed by #9
Closed

merged order wrong? #6

NicoLugil opened this issue Nov 13, 2019 · 10 comments · Fixed by #9

Comments

@NicoLugil
Copy link

I thought this package would be a very nice addition to a mechanism I have in place to specify settings/configurations...but it seems the order in which merged files end up in the final result are giving not the results I would desire.

My settings syntax is as follows:

block:
- rule1:
   setting a
   setting b
   ....
- rule2:
  setting c 
  ...

The rules determine on what settings get applied. It is legal to list the same rule multiple times (that is why it is an array). If a setting is repeated, the 'last one wins' which feels most intuitive and follows what yaml does.

However in the following example:

# base.yaml
---
blockX:
- playground:
    macros:
      M_BASE: ja
      M_REPLACE: to_be_replaced
...
# settings.yaml
---
extends:
  - 'base.yaml'
blockX:
- playground:
    macros:
      M_DERIVE: ja
      M_REPLACE: replaced
...

Gives as result:

---
blockX:
- playground:
    macros:
      M_DERIVE: ja
      M_REPLACE: replaced
- playground:
    macros:
      M_BASE: ja
      M_REPLACE: to_be_replaced
...

In which it is unfortunate that the extended base comes last in the list, as a result when I process the file the macros result in:

M_DERIVE: ja
M_BASE: ja
M_REPLACE: to_be replaced ---> should haven bee 'replaced'

Would it be difficult to put the merged list in the order from base --> derived? (maybe optional)

@NicoLugil
Copy link
Author

BTW: this seems to have a similar cause as #5

@NicoLugil
Copy link
Author

Actually...the example you show on the main page, is not working as written. The outcome I get is:

---
data:
  name: Mr. Superman
  age: 134
  favorites:
  - Raspberrys
  - Bananas
  - Apples
  power: 2000

As you can see some of the items (especially the list of fruits) are in another order compared to the main page. Here they are in 'extensions last'

Probably, often this does not matter, but in my case the order of array entries means something.

@NicoLugil
Copy link
Author

Pull request #8 adds an option to yaml_extend to traverse the tree starting at the base class

@magynhard
Copy link
Owner

@NicoLugil, thank you very much for your issue and pull request.

I would like to provide an option/fix for this requirement and will check your pull request soon.

@magynhard
Copy link
Owner

Hi @NicoLugil,

i want to ensure to understand your problem exactly. Did you expect the following result in your example?

---
blockX:
- playground:
    macros:
      M_BASE: ja
      M_REPLACE: to_be_replaced
- playground:
    macros:
      M_DERIVE: ja
      M_REPLACE: replaced

@NicoLugil
Copy link
Author

Correct!

This is what my pull request gives if you use the option tree_traversal: :postorder

YAML.ext_load_file 'settings.yaml' gives:

{"blockX"=> [{"playground"=>{"macros"=>{"M_DERIVE"=>"ja", "M_REPLACE"=>"replaced"}}}, {"playground"=>{"macros"=>{"M_BASE"=>"ja", "M_REPLACE"=>"to_be_replaced"}}}]}

YAML.ext_load_file 'settings.yaml', tree_traversal: :postorder gives

{"blockX"=> [{"playground"=>{"macros"=>{"M_BASE"=>"ja", "M_REPLACE"=>"to_be_replaced"}}}, {"playground"=>{"macros"=>{"M_DERIVE"=>"ja", "M_REPLACE"=>"replaced"}}}]}

@magynhard
Copy link
Owner

Thank you very much for your reply and PR.

As i figured out, the PR
nangelovaTeladoc#1
Does fix your issue and addresses the multi-file inheritance precedence order as well.

So i prefer using that solution and hope you don't feel bad when i have to reject your suggested solution.

@NicoLugil
Copy link
Author

If it works then that is fine.
However could you run the more elaborate test I provided with a0, a1, ... first to make sure it does give the correct result?

@NicoLugil
Copy link
Author

The one thing I did like about my solution is the use of rubytree.
It as a first step builds a tree.
In the next step it traverses and merges the tree in the desired order.
Rubytree has multiple built-in traversal schemes. If you want to keep that flexibility you might consider having another look at this solution.
It should also fix the multi file order issue.

@magynhard
Copy link
Owner

I will definitely add more test cases, especially on the subject of the merging order.

And I'll reconsider your remarks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants