Skip to content

Conversation

@mk
Copy link
Member

@mk mk commented Nov 26, 2025

No description provided.

Copy link
Member Author

@mk mk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about needing more changes in parser.

:footnotes []
::path [:content -1] ;; private
:text-tokenizers []})
(def empty-doc utils/empty-doc)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have the parser again? Seems to have quite a bit of code duplication and it's not really tested from what I can tell.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in dev and I think no longer used?

:text->id+emoji-fn))))
(dissoc :opts
::path
::id->index))))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update this as well?

@mk mk requested review from borkdude and zampino and removed request for borkdude November 26, 2025 04:35
@borkdude
Copy link
Contributor

@mk I considered doing this, but the reason I didn't proceed, was that "dissoc option from empty-doc to disable option" was a contract from the beginning. Killing that contract will be a breaking change.

;; Node -> {id : String, emoji String}, dissoc from context to opt-out of ids

@borkdude
Copy link
Contributor

@mk @zampino More thoughts:

  • We could check if parse* is called with a document by checking the essential keys :type, :content, ::root. If that is the case, and :text-tokenizers or one of the other two options :disable- and ...emoji-fn is missing, then that counts as disabling text tokenizers, etc.
  • :opts should always override the implicit document options, but how do people disable :emoji-fn, but providing nil? Previously disabling the emoji fn was done by dissoc-ing it from the passed document.
  • I realize now that my check here is wrong:

(if (not (set/superset?
(set (keys ctx))
(set (keys u/empty-doc))))

I should've checked only for essential document keys and not count the implicit option keys since those can be dissoc-ed.

  • I guess dissoc-ing the emoji-fn to opt out also currently doesn't work anymore with parse so that's already a breaking change?

We can also say: screw all of the above backward compatibility changes and we just put a big BREAKING in the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants