-
Notifications
You must be signed in to change notification settings - Fork 603
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
BREAKING(yaml): rename StringifyOptions.noRefs
to StringifyOptions.useAnchors
#5288
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
I tested 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:
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.) |
Oh, yes, let's remove then. |
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 |
How about |
SGTM |
StringifyOptions.noRefs
to StringifyOptions.createRefs
StringifyOptions.noRefs
to StringifyOptions. useAnchors
StringifyOptions.noRefs
to StringifyOptions. useAnchors
StringifyOptions.noRefs
to StringifyOptions.useAnchors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What's changed
The
ParseOptions.noRefs
option has been renamed toParseOptions.useAnchors
. As a result, the polarity of the option has been swapped. In other words,true
behavior has been changed tofalse
behavior, and visa-versa. This only affects users who currently useParseOptions.noRefs
.Motivation
This change makes the option far easier to understand by avoiding a negative-tensed name.
Migration guide
Use
ParseOptions.useAnchors
instead ofParseOptions.noRefs
and swap the polarity of the option.Related
Closes #5124