-
Notifications
You must be signed in to change notification settings - Fork 13
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
@atjson/peritext package for creating and manipulating peritext as a first class data structure #1825
Conversation
} | ||
} | ||
return copy; | ||
} else if (typeof attribute === "string" && ids.has(attribute)) { |
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.
slightly worried about unintended collisions here, but I guess that is extremely unlikely since unstable IDs are UUIDs and stable ids use a reserved construction (M00000001
)
return doc; | ||
} | ||
|
||
function unsafe_parseRange(range: Range) { |
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.
In what sense is this unsafe, that start or end might be NaN
? Would it be better to throw in that case so that it might be handled in a try/catch?
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.
it's unsafe in the sense that it doesn't make any runtime checks that the input is a valid range, so it would behave unpredictably if you pass in an arbitrary string; I don't want to add any checks though, since I expect it to be a pretty hot function in the code and I don't want to add anything that would slow it down
| Peritext | ||
| Peritext[] | ||
| ((block: Block) => string | Peritext | Peritext[]) = "" | ||
): PeritextBuilderStep<Block> { |
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 is nice!
return this.value; | ||
} | ||
|
||
public set<K extends keyof ReturnT>(key: K, value: ReturnT[K]) { |
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.
somewhat surprising to me that we would want to expose this setter. What is the use case?
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.
Oh nvm, was misreading. I thought this was setting the value
. This allows someone to set attributes on the value block or mark. This is 👍
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.
yeah, the only use case I have for it presently is setting the selfClosing
property on blocks, but it would also let you set the attributes in-line if you wanted, or you could set the IDs manually. I don't think there's a like...valid reason to set the parents on a block or the range on a mark but like I don't see a reason to go out of our way to prevent that here
* @see {@link mark} - `slice(children[, attributes])` works like | ||
* `mark(SliceAnnotation, attributes || {refs: []}, children)` | ||
*/ | ||
export function slice( |
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.
nice
} | ||
|
||
return children; | ||
} |
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 is a really nice implementation of this
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.
Very impressive. Besides updating the README this looks great to me. AMAZING work!
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.
Could you add tests for the slice
helper if that is still relevant?
Co-authored-by: Bach Bui <[email protected]>
Adds a suite of functions for creating and manipulating peritext documents directly. The construction functions
mark
,block
, andconcat
are useful for creating properly-nested documents of the sort that appear in XML-like structures. Peritext documents can of course in general have structures where marks overlap each other in arbitrary ways, but this library doesn't provide explicit means to do so.A typical case of creating a document might look like
PeritextBuilderStep
For convenience, several of the functions in this library (particularly
block
,mark
, andslice
) return aPeritextBuilderStep
instead of just a regular peritext document. APeritextBuilderStep
is a peritext document, but when it's created it has an extra value attached to it representing the immediate product of the builder function. This value can be accessed with thegetValue()
method on thePeritextBuilderStep
:Mutation
All of the functions that construct and modify documents have the property that they shallowly copy any peritext documents in their arguments, but directly mutate any blocks and marks on that document. This potentially-surprising behavior is intentional: this way, one can store the results of intermediate build steps and any such references to marks and blocks in the document will remain accurate after applying further build steps. documents are shallowly-copied in order for the behavior of functions like
concat
(which take multiple document objects and so could only mutate one of them anyway) to be consistent with the behavior of functions likegroupChildren
(which could in principle simply mutate the input document directly)