-
-
Notifications
You must be signed in to change notification settings - Fork 150
feat: Expose isMergeKey and MERGE_KEY constant
#651
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
base: main
Are you sure you want to change the base?
Conversation
src/schema/yaml-1.1/merge.ts
Outdated
| // later mapping nodes. -- http://yaml.org/type/merge.html | ||
|
|
||
| const MERGE_KEY = '<<' | ||
| export const MERGE_KEY: unique symbol = Symbol.for('<<') |
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.
Probably best to roll back this change.
Currently, main has changes that will only get released as a v3 release of the library, so if you want/need isMergeKey to be available on v2, this PR needs to not include breaking changes.
For v3, I intend to be much stricter on the values that can be put in a document, and so for something like merge keys, end up with an instanceof check.
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.
for something like merge keys, end up with an
instanceofcheck.
I thought about instanceof, but that operator doesn’t work on Symbols, which seem like the best fit for merge keys, surely?
this PR needs to not include breaking changes.
Would exporting MERGE_KEY as a shared Symbol even be a breaking change, given that it seems to only currently exist within this file?
Probably best to roll back this change.
Have rolled back to just exporting MERGE_KEY as string for now/v2…
isMergeKey as utilisMergeKey and MERGE_KEY constant
| // later mapping nodes. -- http://yaml.org/type/merge.html | ||
|
|
||
| const MERGE_KEY = '<<' | ||
| export const MERGE_KEY = '<<' |
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.
As I mention in my previous review, I intend to make type checking stricter in v3. To that end, we shouldn't be exposing MERGE_KEY, but providing and enforcing the use of a new Merge class for this purpose.
| export const MERGE_KEY = '<<' | |
| const MERGE_KEY = '<<' |
| default: 'key', | ||
| tag: 'tag:yaml.org,2002:merge', | ||
| test: /^<<$/, | ||
| test: new RegExp(`^${MERGE_KEY}$`), |
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.
This isn't really making the code more legible.
| test: new RegExp(`^${MERGE_KEY}$`), | |
| test: /^<<$/, |
Trying to use
isMergeKeyfor something… Is there a reason it isn’t exposed alongsideisScalaretc? Also, seems likeMERGE_KEYshould be a shared symbol, no?