Add support for merge tags

Wikiwix commented 2 years ago

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 commented 2 years ago

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..

Wikiwix commented 1 year ago

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: https://github.com/yaml/libyaml/issues/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 commented 1 year ago

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:

https://github.com/flutter/flutter/pull/119852/commits/1abf4f4927c0767709b13e6d2bacb62769a8cb2c

jonasfj commented 3 months ago

(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 :see_no_evil:

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 commented 3 months ago

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.

dart-lang / yaml

Add support for merge tags #121