denoland / deno_std

The Deno Standard Library
https://jsr.io/@std
MIT License
2.83k stars 582 forks source link

BREAKING(yaml): rename `StringifyOptions.noRefs` to `StringifyOptions.useAnchors` #5288

Closed iuioiua closed 2 days ago

iuioiua commented 3 days ago

What's changed

The ParseOptions.noRefs option has been renamed to ParseOptions.useAnchors. As a result, the polarity of the option has been swapped. In other words, true behavior has been changed to false behavior, and visa-versa. This only affects users who currently use ParseOptions.noRefs.

Motivation

This change makes the option far easier to understand by avoiding a negative-tensed name.

Migration guide

Use ParseOptions.useAnchors instead of ParseOptions.noRefs and swap the polarity of the option.

import { parse } from "@std/yaml/parse";

const yamlString = `fruits:
  - Apple
  - Banana
  - Cherry`;

- parse(yamlString, { noRefs: false });
+ parse(yamlString, { useAnchors: true });

Related

Closes #5124

codecov[bot] commented 3 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.80%. Comparing base (fcc59f2) to head (b5ce607). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #5288 +/- ## ======================================= Coverage 95.80% 95.80% ======================================= Files 458 458 Lines 37852 37852 Branches 5562 5562 ======================================= Hits 36264 36264 Misses 1548 1548 Partials 40 40 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kt3k commented 3 days ago

I tested noRefs option, but it doesn't seem working in std/yaml.

import { stringify } from "./yaml/mod.ts";
import * as yaml from "npm:js-yaml";

const a = {
  foo: {
    foo: {
      foo: {
      },
    },
  },
};
console.log(stringify([a, a], { noRefs: false }));
console.log(stringify([a, a], { noRefs: true }));
console.log(yaml.dump([a, a], { noRefs: false }));
console.log(yaml.dump([a, a], { noRefs: true }));

This prints:

- foo:
    foo:
      foo: {}
- foo:
    foo:
      foo: {}

- foo:
    foo:
      foo: {}
- foo:
    foo:
      foo: {}

- &ref_0
  foo:
    foo:
      foo: {}
- *ref_0

- foo:
    foo:
      foo: {}
- foo:
    foo:
      foo: {}

I suggest we should rather remove this option. What do you think?

(In my opinion, reference (alias) is not well understood feature of yaml. I think people would want to avoid them by default. The current behavior doesn't create reference (alias), and it is probably aligned to most users' expectation.)

iuioiua commented 3 days ago

Oh, yes, let's remove then.

kt3k commented 3 days ago

Ah, wait. The feature seems working with 0.224.3, but it's gone in 1.0.0-rc.1. It seems to get broken somewhere between 0.224.3 - 1.0.0-rc.1

kt3k commented 3 days ago

How about useAnchors instead? 'reference' is not an official yaml term for it. Also 'create' sounds a bit strange to me. It reduces the duplicate occurrence of the same objects, instead of creating something

iuioiua commented 3 days ago

SGTM