Skip to content
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

feat!: map known code block languages to respective file extensions #246

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

DMartens
Copy link
Contributor

Fixes #245.
I researched common language codes for javascript and markdown which are not the same as the file extension.
There may be additional ones.
The current tests had test cases for the language codes javascript and node already, so I adjusted them and added a case to make sure the mapping from code block language to file extension is done after the code block language normalization.
This may be a breaking change as additional code blocks may be linted.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together. I agree, this looks like a breaking change, so adding the label.

It actually looks like this change might have broken the lint task, can you take a look at that?

Otherwise, just a few suggestions throughout.

@@ -115,7 +115,7 @@ describe("processor", () => {
"```js",
"backticks",
"```",
"~~~javascript",
"~~~js",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would make sense to leave this as "javascript" to ensure it gets translated into .js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are already the preexisting test cases "should find code fences with javascript info string" and "should find code fences with node info string" which test the mapping. As such I changed it in the other test cases.

tests/lib/processor.js Outdated Show resolved Hide resolved
tests/lib/processor.js Show resolved Hide resolved
@DMartens
Copy link
Contributor Author

The lint task was failing as one of the mappings also enabled linting the nested code blocks nested in markdown.
As I thought all the detected lint errors are irrelevant for demonstrating the plugin, I added them to the existing rule ignores.

@DMartens
Copy link
Contributor Author

DMartens commented Apr 8, 2024

I removed the code block language mapping from node to js as mentioned in the issue.
Should I add documentation for this case such as setting parserOptions: { sourceType: 'commonjs' }?

@nzakas
Copy link
Member

nzakas commented Apr 8, 2024

I think at this point we don't need to do anything to cater to CommonJS, specifically. I'm not sure what node is meant to indicate in Markdown anyway.

@nzakas
Copy link
Member

nzakas commented Apr 8, 2024

The lint task was failing as one of the mappings also enabled linting the nested code blocks nested in markdown. As I thought all the detected lint errors are irrelevant for demonstrating the plugin, I added them to the existing rule ignores.

Can you explain this a bit more? I'm not quite sure I follow what happened here. On HEAD linting passes as-is, so I don't just want to disable rules without understanding what changed.

@DMartens
Copy link
Contributor Author

DMartens commented Apr 8, 2024

One of the added mappings from code block language was from markdown to md.
This is also the case in the README.md section "What gets linted").. As such the processor is called recursively which was not the case before (just forgot to run the linter on all files).
I ignored the rules such as the console.log (no-console) or function without jsdoc (jsdoc/require-jsdoc) in the nested code blocks are for what I think are demonstration purposes.
The same reason as why no-var for markdown code block was disabled before this PR.

nzakas
nzakas previously approved these changes Apr 10, 2024
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. This looks good to me now.

@nzakas nzakas changed the title feat: map known code block languages to respective file extensions feat!: map known code block languages to respective file extensions Apr 10, 2024
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Sorry, one last thing: can you please update the README to explain how this works? (There's already a section about how filenames are determined, so expanding on that would be great.)

README.md Outdated Show resolved Hide resolved
nzakas
nzakas previously approved these changes Apr 15, 2024
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! Just leaving open to see if other wants to review before merging.

Copy link
Member

@fasttime fasttime left a comment

Choose a reason for hiding this comment

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

Just a note about the docs, otherwise LGTM.

README.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Merge Candidates
Development

Successfully merging this pull request may close these issues.

feat: Support different names for code block languages
3 participants