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

Add support for merge tags #121

Open
Wikiwix opened this issue Oct 31, 2021 · 5 comments · May be fixed by #151 or #158
Open

Add support for merge tags #121

Wikiwix opened this issue Oct 31, 2021 · 5 comments · May be fixed by #151 or #158
Labels
type-enhancement A request for a change that isn't a bug

Comments

@Wikiwix
Copy link

Wikiwix commented Oct 31, 2021

The yaml package (in v2.3.0 at least) does not support !!merge tags.

For implicit instances (with only a map key of <<) it gets just interpreted as a !!str node.
If the !!merge tag is given explicitly an exception is thrown.

import 'package:yaml/yaml.dart';

const yamlImplicitMerge = '''---
test: &anchor
  ancValue1: 1
  ancValue2: 2
ancUse:
  someOther: 3
  <<: *anchor
''';

const yamlExplicitMerge = '''---
test: &anchor
  ancValue1: 1
  ancValue2: 2
ancUse:
  someOther: 3
  !!merge <<: *anchor
''';

void main(List<String> arguments) {
  <String Function()>[
    () => 'implicit merge gets ignored:',
    () => loadYaml(yamlImplicitMerge),
    () => '————————————————————————————————',
    () => 'Explicit merge throws exception:',
    () => loadYaml(yamlExplicitMerge),
  ].forEach((lazyString) => print(lazyString()));
}

outputs

> dart run .\bin\yaml_test.dart
implicit merge gets ignored:
{test: {ancValue1: 1, ancValue2: 2}, ancUse: {someOther: 3, <<: {ancValue1: 1, ancValue2: 2}}}
————————————————————————————————
Explicit merge throws exception:
Unhandled exception:
Error on line 7, column 3: Undefined tag: tag:yaml.org,2002:merge.

7 │   !!merge <<: *anchor
  │   ^^^^^^^^^^

#0      Loader._parseByTag (package:yaml/src/loader.dart:201:9)
#1      Loader._loadScalar (package:yaml/src/loader.dart:120:14)
#2      Loader._loadNode (package:yaml/src/loader.dart:85:16)
#3      Loader._loadMapping (package:yaml/src/loader.dart:165:17)
#4      Loader._loadNode (package:yaml/src/loader.dart:89:16)
#5      Loader._loadMapping (package:yaml/src/loader.dart:166:19)
#6      Loader._loadNode (package:yaml/src/loader.dart:89:16)
#7      Loader._loadDocument (package:yaml/src/loader.dart:65:20)
#8      Loader.load (package:yaml/src/loader.dart:57:20)
#9      loadYamlDocument (package:yaml/yaml.dart:69:25)
#10     loadYamlNode (package:yaml/yaml.dart:54:5)
#11     loadYaml (package:yaml/yaml.dart:41:5)
#12     main.<anonymous closure> (file:///G:/My%20Drive/dev/yaml_test/bin/yaml_test.dart:27:11)   
#13     main.<anonymous closure> (file:///G:/My%20Drive/dev/yaml_test/bin/yaml_test.dart:28:31)   
#14     List.forEach (dart:core-patch/growable_array.dart:403:8)
#15     main (file:///G:/My%20Drive/dev/yaml_test/bin/yaml_test.dart:28:5)
#16     _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:281:32)
#17     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)
@jonasfj
Copy link
Member

jonasfj commented Sep 8, 2022

I might want to ask:

(A) What does https://github.com/yaml/libyaml do?
(B) What do other implementations do https://matrix.yaml.info/ ?
(C) What does the YAML 1.2 spec say? https://yaml.org/spec/1.2.2/

Mostly, because I'm not sure we should add features that aren't widely supported and needed.
For most use-cases YAML is just a configuration file that contains some JSON objects..

@jonasfj jonasfj added type-enhancement A request for a change that isn't a bug needs-info Additional information needed from the issue author labels Sep 8, 2022
@Wikiwix
Copy link
Author

Wikiwix commented Sep 17, 2022

I might want to ask:

(A) What does https://github.com/yaml/libyaml do?
(B) What do other implementations do https://matrix.yaml.info/ ?
(C) What does the YAML 1.2 spec say? https://yaml.org/spec/1.2.2/

Mostly, because I'm not sure we should add features that aren't widely supported and needed. For most use-cases YAML is just a configuration file that contains some JSON objects..

(A) libyaml does not support the merge tag either: yaml/libyaml#168
(B) As far as I can see there is no test for the merge tag
(C) The spec does not provide a Schema that defines the merge Tag. it is an extension documented at yaml.org though not a requirement for YAML 1.2 compliance.

@jmagman
Copy link

jmagman commented Feb 2, 2023

This feature would be great for managing config files with shared properties. For example in the Flutter plugins repository we can create aliases and merge the nodes in to prevent duplication:

tool_setup_template: &TOOL_SETUP_TEMPLATE
  tool_setup_script:
    - .ci/scripts/prepare_tool.sh
...
flutter_upgrade_template: &FLUTTER_UPGRADE_TEMPLATE
  upgrade_flutter_script:
    - flutter doctor -v
  << : *TOOL_SETUP_TEMPLATE 
...
task:
  << : *FLUTTER_UPGRADE_TEMPLATE
  gke_container:
    dockerfile: .ci/Dockerfile
...

I've simplified it copied here but you can see how much duplication this saves:
https://github.com/flutter/plugins/blob/9302d87ee5452344878eed7beb1835c959ab1d27/.cirrus.yml#L9

I tried to do something similar in the Flutter repo CI config file, which is using this dart yaml package, but I can't merge the properties because it isn't supported:

flutter/flutter@1abf4f4

@Zekfad Zekfad linked a pull request Sep 14, 2023 that will close this issue
1 task
@farcaller farcaller linked a pull request Mar 16, 2024 that will close this issue
@jonasfj
Copy link
Member

jonasfj commented Jun 7, 2024

(C) The spec does not provide a Schema that defines the merge Tag. it is an extension documented at yaml.org though not a requirement for YAML 1.2 compliance.

I also found a decent write up here: https://ktomk.github.io/writing/yaml-anchor-alias-and-merge-key.html
Which also suggests the feature is "born deprecated".

I have two questions:

  • (A) Are we sure we want to add this feature?
    • merge tags not in YAML 1.2 specification
    • libyaml doesn't support merge tags
    • It's unclear what other YAML implementations do (it's not covered by yaml-test-suite)
  • (B) Can this be done through post-processing?
    • Could we include a resolvedMergeTags function in package:yaml that consumers could opt to use or not use?
    • Can the community ship a package:yaml_merge_tags with a function that takes YamlNode and resolves all merge tags in a YamlNode structure?

To be fair, I'm biased, because I mainly use package:yaml for simple configuration files (like pubspec.yaml, analysis_options.yaml, dartdoc_options.yaml, build.yaml, etc).
And we maintain tooling for updating such files, including package:yaml_edit.

And for configuration files, features such as:

  • aliases / anchor
  • tags
  • merge tags
  • maps with non-string keys 🙈

are often the source of bugs and unnecessary complexity.

Aliases and anchors are cool, but our config files are often fairly small, do you really need them? Isn't it better to duplicate the config?

Because aliases and anchors are supported package:yaml_edit actually scans the entire YamlNode tree and compares all nodes to see if any appear more than once. If so, it will refuse to modify anything related to those nodes.

Adding support for merge keys would be another feature that would be extremely hard to support in tooling that edits YAML files.

It would probably also result in weird error messages, because SourceSpans would be weird in some scenarios.

So personally, unless there is a really compelling reason. Unless all other YAML packages do implement this feature. And unless it can't be implemented in a post-processing step, I'd suggest we just don't support this in package:yaml.

I suspect merge tags could be supported through post-processing, which could live happily in a community owned package:yaml_merge_tags. If anyone wants to own such a package, document it, etc. I think we could add link to in the README.md :D

@Zekfad
Copy link

Zekfad commented Jun 7, 2024

It can be done in post-process step but requires knowledge about anchors. If these are available than it's ok to implement one.
Besides to comply with extension spec we need to ensure no duplicate of anchor which would make merge resolution itself a double step process.
Implementing it in reader is not very hard, and edit tooling can either view resolved yaml, or just don't support anchors for edit. This feature is a convenience for manual text editing yaml, not for software.

@github-actions github-actions bot removed the needs-info Additional information needed from the issue author label Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
5 participants
@jonasfj @jmagman @Wikiwix @Zekfad and others