fix:ambiguity around extensions - #3787
Conversation
|
@DewminK is attempting to deploy a commit to the MarkedJS Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @DewminK, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to enhance the clarity and precision of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does a great job of clarifying the different types of extensions in marked by renaming the extensions option to tokenizerAndRendererExtensions. The changes are applied consistently across the documentation, source code, and tests. Maintaining backward compatibility by supporting the old property name is also handled well.
I have one suggestion for the new test file to improve test isolation, which will make the tests more robust.
Also, there's a small typo in the pull request title: 'ambuguity' should be 'ambiguity'.
| import { marked } from './src/marked.ts'; | ||
|
|
||
| // Test that both property names work | ||
| const testExtension = { | ||
| name: 'test', | ||
| level: 'inline', | ||
| start(src) { return src.indexOf('TEST'); }, | ||
| tokenizer(src) { | ||
| const match = src.match(/^TEST/); | ||
| if (match) { | ||
| return { | ||
| type: 'test', | ||
| raw: match[0], | ||
| text: 'TESTED' | ||
| }; | ||
| } | ||
| }, | ||
| renderer(token) { | ||
| return `<span class="test">${token.text}</span>`; | ||
| } | ||
| }; | ||
|
|
||
| // Test old property name (should still work) | ||
| console.log('Testing old property name (extensions):'); | ||
| try { | ||
| marked.use({ extensions: [testExtension] }); | ||
| console.log('✓ Old property name works'); | ||
| console.log('Result:', marked.parse('This is TEST content')); | ||
| } catch (e) { | ||
| console.log('✗ Old property name failed:', e.message); | ||
| } | ||
|
|
||
| // Reset marked | ||
| marked.setOptions(marked.getDefaults()); | ||
|
|
||
| // Test new property name | ||
| console.log('\nTesting new property name (tokenizerAndRendererExtensions):'); | ||
| try { | ||
| marked.use({ tokenizerAndRendererExtensions: [testExtension] }); | ||
| console.log('✓ New property name works'); | ||
| console.log('Result:', marked.parse('This is TEST content')); | ||
| } catch (e) { | ||
| console.log('✗ New property name failed:', e.message); | ||
| } No newline at end of file |
There was a problem hiding this comment.
While the current test correctly validates both property names by resetting the global marked instance, this approach of mutating and resetting global state can make tests brittle and harder to maintain. A better practice for test isolation is to use separate Marked instances for each test case. This avoids side effects between tests and makes them more robust. The suggested change refactors the test to use this approach, and also updates the import statement accordingly.
import { Marked } from './src/marked.ts';
// Test that both property names work
const testExtension = {
name: 'test',
level: 'inline',
start(src) { return src.indexOf('TEST'); },
tokenizer(src) {
const match = src.match(/^TEST/);
if (match) {
return {
type: 'test',
raw: match[0],
text: 'TESTED'
};
}
},
renderer(token) {
return `<span class="test">${token.text}</span>`;
}
};
// Test old property name (should still work)
console.log('Testing old property name (extensions):');
try {
const markedOld = new Marked({ extensions: [testExtension] });
console.log('✓ Old property name works');
console.log('Result:', markedOld.parse('This is TEST content'));
} catch (e) {
console.log('✗ Old property name failed:', e.message);
}
// Test new property name
console.log('\nTesting new property name (tokenizerAndRendererExtensions):');
try {
const markedNew = new Marked({ tokenizerAndRendererExtensions: [testExtension] });
console.log('✓ New property name works');
console.log('Result:', markedNew.parse('This is TEST content'));
} catch (e) {
console.log('✗ New property name failed:', e.message);
}
DewminK
changed the title
UziTech
left a comment
UziTech
left a comment
There was a problem hiding this comment.
I would prefer leaving the extension option as extensions.
Updating the docs to use plugin instead of marked extension seems like a good idea.
|
I'm going to close this as stale. If you would like to continue working on this feel free to reopen it with conflicts and comments resolved. |
2 participants
Marked version:
Markdown flavor: Markdown.pl|CommonMark|GitHub Flavored Markdown|n/a
Description
Contributor
Committer
In most cases, this should be a different person than the contributor.