[Doc][README] trouble shooting section should use require if the examples in README are JavaScript
#15746
Comments
jeremymeng
added
the
Client
|
@deyaaeldeen Do you think we can have a linter rule for our package readmes to catch such cases? |
ramya-rao-a
added
eslint plugin
EngSys
|
@ramya-rao-a if you mean just checking for import syntax, I think we can. But if we want to make sure the code snippet as a whole runs fine, this is another story. |
|
Yes agreed. I meant to only catch the import syntax |
ramya-rao-a
added
the
help wanted
|
Code pointers for anyone wanting to fix this:
|
|
eslint might not be able to work on markdown files? |
|
@jeremymeng I think something like this could work: https://github.com/eslint/eslint-plugin-markdown |
|
I would like to work on this as suggested by @deyaaeldeen though I think I need some clarification. I am aware that I should change all |
|
@WeiJun428 the idea is to make sure the trouble shooting section in every package's readme file uses CommonJS module syntax so that Node.JS users could run the code without issues. For example, see Text Analytics's troubleshooting section, it uses ES modules, but we would like that to be rewritten to use CommonJS syntax (require) instead. |
|
@jeremymeng I think it makes sense to be consistent about the module syntax across the whole readme file, do you think there're any downsides for using CommonJS throughout? /cc @xirzec @witemple-msft |
I prefer CommonJS too. Ideally customers should be able to copy and paste code snippets without running into problems. |
|
I agree that we should switch to CommonJS for code snippets (at least until NodeJS more wholeheartedly embraces import syntax) |
|
@jeremymeng @xirzec thanks! @WeiJun428 ok let's generalize this issue to be about ensuring using |
|
Hi, I would like to get some clarification on where to incorporate |
|
@WeiJun428 thanks for following up! |
|
Thanks @deyaaeldeen for your answer! Now, I am trying to use so that |
|
@WeiJun428 You're right that current npm linting scripts ignore the README.md files so they will have to be updated. Instead of adding all .md files, let's add README.md only to the linting commands: |
…ript code blocks (#20408) This is a PR that is intended to solve issue #15746. Summary of What I did: - ran `rush add -p eslint-plugin-markdown --dev` in `common\tools\eslint-plugin-azure-sdk` - modified `common\tools\eslint-plugin-azure-sdk\src\configs\azure-sdk-base.ts`: - included the `eslint-plugin-markdown` plugin, - added `override` configuration that splits typescript and javascript linting. - used `no-restricted-import` to inhibit ES6 import usage. - added `README.md` as target and removed `--ext` option of lint script in `sdk\textanalytics\ai-text-analytics\package.json` and fixed the existing error.
|
@WeiJun428 I created a feature branch |
|
@jeremymeng I didn't see the feature branch |
|
@WeiJun428 This could be a permission issue. I will try to find a solution, meanwhile you can work on your own branch. |
|
@WeiJun428 It should be available now at https://github.com/Azure/azure-sdk-for-js/tree/feature/enable-linting-readme |
|
@jeremymeng I wonder if there is a good way to track the progress of enable linting readme in all sdk? Something like a list or checkbox of all to-be-fixed sdk? Should we enable it in tools such as |
|
@WeiJun428 good question! We don't have inventory of non-generated SDK libraries. Let me double check to see if I can find anything helpful. The main goal of this work is helping SDK customers who copy and paste code. It doesn't really apply to internal tools so we don't need to enable this rule for them. |
jeremymeng
added
the
Impact++
|
@weiThe list below should be pretty close
|
|
Thanks! @jeremymeng. I have tried to create a PR targeting that branch but I noticed that it also includes 12 other commits perhaps due to difference between the versions of repo that I fetch and the feature branch itself. Would that be an issue? |
|
@WeiJun428 Let me see if I can update the feature branch to also include those changes. |
jeremymeng
removed
the
Impact++
|
@jeremymeng is this issue resolved? If so, can we close it? |
|
Hi @jeremymeng, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support. |
We have seen in UX Study that participants copied and pasted the code into a NodeJS javascript file but the code didn't work because it's using
importwhich doesn't work in default NodeJS project. We should update those to useconst { setLogLevel } = require("@azure/logger");The text was updated successfully, but these errors were encountered: