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

Chained merges <<: don’t work #197

Closed
danielbayley opened this issue Dec 1, 2024 · 12 comments
Closed

Chained merges <<: don’t work #197

danielbayley opened this issue Dec 1, 2024 · 12 comments

Comments

@danielbayley
Copy link
Contributor

For example:

one: &a
  a: 1

two: &b
  <<: *a
  b: 2

three:
  <<: *b
  c: 3

should produce:

{
  "one": {
    "a": 1
  },
  "two": {
    "a": 1,
    "b": 2
  },
  "three": {
    "a": 1,
    "b": 2,
    "c": 3
  }
}

but instead, produces:

{
  "one": {
    "a": 1
  },
  "two": {
    "a": 1,
    "b": 2
  },
  "three": {
    "<<": {
      "a": 1
    },
    "b": 2,
    "c": 3
  }
}
@ingydotnet
Copy link
Member

Good catch. Fixed in ffdf7f5.
Pushed to devel branch.
You can install ys from that branch with:

make install-ys PREFIX=</some/path/with/bin/in/PATH>  # defaults to $HOME/.local

Planning to release 0.1.86 tonight.
Looking at your PR #196 next.

@ingydotnet
Copy link
Member

Fix ffdf7f5 released in 0.1.86

@danielbayley
Copy link
Contributor Author

danielbayley commented Dec 3, 2024

Nice one @ingydotnet, one thing remains though—so while this works now:

one: &a
  a: 1

two: &b
  <<: *a
  b: 2

three:
  <<: *b

(although the order of keys is reversed—which is probably a bug?)

{
  "one": {
    "a": 1
  },
  "two": {
    "b": 2,
    "a": 1
  },
  "three": {
    "c": 3,
    "b": 2,
    "a": 1
  }
}

This does not:

one: &a
  a: 1

two: &b
  <<: *a
  b: 2

three: *b
{
  "one": {
    "a": 1
  },
  "two": {
    "b": 2,
    "a": 1
  },
  "three": {
    "<<": {
      "a": 1
    },
    "b": 2
  }
}

@ingydotnet
Copy link
Member

The key order is right afaik.
It's based on this old thing https://yaml.org/type/merge.html
Which says that you start with the mapping minus the << key, and then add keys from << if they don't exist.

Re the failing case, I suspected that when I fixed it but didn't push past your reported problem because I didn't have the time for it. I will take a look this week.

Please keep the issues coming.

@ingydotnet ingydotnet reopened this Dec 3, 2024
@ingydotnet
Copy link
Member

Looking at this now.

one: &a
  a: 1

two: &b
  <<: *a
  b: 2

three: *b

compiles to:

{"one" (_& 'a {"a" 1}),
 "two" (+merge (_& 'b {"<<" (_* 'a), "b" 2})),
 "three" (_* 'b)}

I need to have the _* function look for the merge key and do the merge.

There's a little problem though. At this point we don't know if the key was << '<<' or "<<".
The << has to be unquoted to be a YAML merge key.

I have an idea. I'm going to make the compiler produce

{"one" (_& 'a {"a" 1}),
 "two" (+merge (_& 'b {:-<< (_* 'a), "b" 2})),
 "three" (_* 'b)}

The :-<< is a valid Clojure keyword but something that will never show up as a YAML or YS data key.
I'm not even sure keywords make sense at all in data mode, but using a - in front just to be safe (in case it makes sense down the road).

@ingydotnet
Copy link
Member

@danielbayley this is fixed in the devel branch now.
I'll likely release this on Sunday, but if you want to test things out you can install easily with: make install-ys.
It takes a minute or 2 to compile but doesn't require any deps.

@ingydotnet
Copy link
Member

44f7fd5 is the fix commit, fwiw.

It was quite a refactoring of how this works but I think it makes it more solid now.

@ingydotnet
Copy link
Member

Fixed released in 0.1.87

@danielbayley
Copy link
Contributor Author

danielbayley commented Dec 20, 2024

Fixed released in 0.1.87

Nice one @ingydotnet! Will check it out…

Although, with Homebrew being my preferred install method—actually the only method I allow as a strict self-imposed rule for everything on my machine—I'm wondering if there should be some CI step to automatically bump the formula version on release?

Edit: I notice there already is: the brew-update util (which, FYI, could also live as an external command over in the Homebrew tap in a cmd directory…), I think it just needs wiring up with an Actions workflow to trigger on: release

@ingydotnet
Copy link
Member

I barely know what I'm doing with homebrew. Any help you can offer would be great.

https://matrix.to/#/@ingy:yaml.io is a good way to chat with me directly if you want...

@danielbayley
Copy link
Contributor Author

danielbayley commented Dec 21, 2024

I barely know what I'm doing with homebrew. Any help you can offer would be great.

I can take a look at the setup… As it stands, the brew-update util kept failing for me, so I opened yaml/homebrew-yamlscript#1 as a manual update.

https://matrix.to/#/@ingy:yaml.io is a good way to chat with me directly if you want...

Cool, I'm really hesitant to sign up for yet another account for anything though — according to 1Password I already have well over 400! Trying to cut down…

@danielbayley
Copy link
Contributor Author

I barely know what I'm doing with homebrew. Any help you can offer would be great.

@ingydotnet Sure, check out #207 and yaml/homebrew-yamlscript#3!

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

No branches or pull requests

2 participants