-
Notifications
You must be signed in to change notification settings - Fork 3.5k
BREAKING CHANGE: add checkbox token to tasks #3755
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
/gemini review |
|
@gemini-code-assist review |
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.
Code Review
This pull request introduces a significant and beneficial refactoring by moving token creation logic from the parser to the tokenizer. The main driver for this change is the introduction of a dedicated checkbox token for task list items, which allows these tokens to be processed by walkTokens like other tokens. This change also simplifies the parser by removing special handling for text tokens, as they are now appropriately converted to paragraph tokens within the tokenizer for loose lists. The code is well-structured, and the breaking changes are justified by the architectural improvements. I have one minor suggestion to improve code clarity.
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.
Code Review
This pull request introduces a significant and beneficial refactoring by moving checkbox tokenization out of the parser and into the tokenizer. This makes the new checkbox token accessible to walkTokens and processAllTokens, improving extensibility. The simplification of the listItem renderer and the changes to how text tokens are handled in lists are great improvements for code clarity and maintenance.
I've found a few areas for improvement:
- A potential bug in
Parser.tswherethis.rendereris used instead of therendererparameter, which could cause issues withTextRenderer. - A couple of opportunities in
Tokenizer.tsto improve code clarity and type safety.
Overall, this is a solid contribution that improves the architecture of tokenization in marked. Addressing the points in my review will make it even more robust.
|
/gemini review |
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.
Code Review
This pull request is a significant and beneficial refactoring. It introduces a dedicated checkbox token and moves the logic for handling task list items from the renderer to the tokenizer. This simplifies the renderer and makes the token stream more expressive, which is a great improvement for extensibility, especially for walkTokens. The changes to handle loose list text as paragraph tokens and simplifying the top-level text token handling in the parser are also solid improvements that make the parsing process more consistent.
The code is well-structured, and the tests have been updated thoroughly to reflect the changes. I have one point of feedback regarding token consistency in the tokenizer, which could prevent potential issues with extensions.
|
/gemini review |
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.
Code Review
This pull request introduces a significant and beneficial refactoring by adding a dedicated checkbox token for task lists. Moving the token creation logic from the parser and renderer into the tokenizer is a strong architectural improvement that simplifies the codebase and improves maintainability. This change makes the token stream more consistent, which is particularly valuable for extensions that rely on walkTokens. The breaking changes are well-documented and are a reasonable trade-off for the cleaner implementation. The code is well-structured, and the tests are updated accordingly. I've found one minor inconsistency, but overall, this is an excellent enhancement.
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.
Code Review
This pull request introduces a significant and beneficial refactoring by making task list checkboxes into first-class tokens. This moves complex logic from the renderer to the tokenizer, simplifying the rendering process and making the token stream more consistent and extensible. The changes, while breaking, are well-documented and improve the overall architecture. The implementation is solid, with corresponding updates to tests to ensure correctness. I have one minor suggestion for code consistency.
|
/gemini review |
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.
Code Review
This pull request is a significant and beneficial refactoring. It moves the responsibility of creating checkbox tokens and handling loose list item text from the parser and renderer into the tokenizer. This is a great architectural improvement that makes the parser and renderer simpler and more predictable. The introduction of a dedicated checkbox token is a clean solution. The breaking changes are well-documented and justified by the improvements in code structure and maintainability. I have one suggestion regarding type safety in the tokenizer, but overall, this is excellent work.
8229f4f to
7e9de83
Compare
|
@UziTech is attempting to deploy a commit to the MarkedJS Team on Vercel. A member of the Team first needs to authorize it. |
7e9de83 to
f3e834d
Compare
# [17.0.0](v16.4.2...v17.0.0) (2025-11-07) ### Bug Fixes * only create tokens inside tokenizers ([#3755](#3755)) ([7b19231](7b19231)) ### BREAKING CHANGES * - Change how consecutive text tokens work in lists - Simplify listItem renderer - Checkbox token is added in list tokenizer - Checkbox token add type and raw property - Change loose list text tokens to paragraph type in the list tokenizer
Marked version: 16.2.1
Description
fixes #3754 (reply in thread)
Move token creation out of parser. Any token currently created in the parser will not go through
walkTokensorprocessAllTokens.To do this I have added a checkbox token to task list items and set text tokens in loose lists to paragraph type tokens since they end up that way in the parser currently.
Breaking changes:
typeandrawpropertyContributor
Committer
In most cases, this should be a different person than the contributor.