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

Lint inline Markdown JS #2524

Closed
XhmikosR opened this issue Sep 7, 2019 · 15 comments
Closed

Lint inline Markdown JS #2524

XhmikosR opened this issue Sep 7, 2019 · 15 comments

Comments

@XhmikosR
Copy link
Contributor

XhmikosR commented Sep 7, 2019

This will ensure consistency. We should try doing this after #2523 and use the same config as upstream.

@nschonni
Copy link
Member

Looks like it can be done with the markdown plugin https://github.com/eslint/eslint-plugin-markdown
I tried it with a --fix locally, but didn't PR anything since I find the "standard" config kind of makes a mess of the code

@XhmikosR
Copy link
Contributor Author

Yup, this should go after #2523

@carterbancroft
Copy link

What's the status of this issue, is it still something that needs to be worked on?

If so, can you share an example or two of what needs linting?

@XhmikosR
Copy link
Contributor Author

IMO yeah. Every .md file needs to be linted.

@carterbancroft
Copy link

Roger. I'm new here and just trying to get more context so please bear with me.

So, is there no linting on markdown files at all right now? From the code (and your PR here: https://github.com/nodejs/nodejs.org/pull/2557/files) it looks like there are markdown linting rules in place and linting scripts in package.json. Is this not happening?

Or, is it just the inline javascript within .md files that needs linting? If so any chance you could link me to a specific code block of something that needs linting as an example.

@XhmikosR
Copy link
Contributor Author

This is issue is for linting JS code in the markdown files. You can search the locale files for the following:

```js
#or
```javascript

There are hundreds of instances. Ideally we should use the same linting used in the main repo so that we are consistent, so this might need to be done along with #2523

@carterbancroft
Copy link

OK gotcha, thanks.

So am I understanding correctly that there are still more linting rules from the node repo that should be implemented in this repo? I made the (probably faulty) assumption that this was the last step since this is the only remaining issue referenced in #2523. The rest are merged PRs.

@XhmikosR
Copy link
Contributor Author

TBH, I'm not aware of the internals in the upstream repo. I know they use eslint while here we use standard, which is also using eslint under the hood.

@carterbancroft
Copy link

I see, so it sounds like #2523 should be figured out before there's any movement on this one. Maybe I should move conversation over there to try to get some context on that?

@XhmikosR
Copy link
Contributor Author

Not sure which one should be tackled first.

/CC @Trott @MylesBorins

@Trott
Copy link
Member

Trott commented Jan 15, 2020

I don't know that it matters which one is done first, although I guess I'd default to doing #2523 first.

@carterbancroft
Copy link

Thanks, and just to clarify, this issue and #2523 won't be made moot with the website redesign, yeah?

@Trott
Copy link
Member

Trott commented Jan 15, 2020

Thanks, and just to clarify, this issue and #2523 won't be made moot with the website redesign, yeah?

They might be, but to be honest, it's not clear to me that @nodejs/website-redesign hasn't completely stalled. It might be unlikely to ever complete. Judging from https://github.com/nodejs/nodejs.dev/tree/master/meetings, they haven't had a meeting in almost six months.

(If that initiative is still going, great! But if not, my favored Plan B would be to iterate on this site and ditch the idea of a full-blown redesign and re-launch. No opposition to a full redesign. But I don't want something that's not going to happen to end up discouraging work here that could be valuable.)

@carterbancroft
Copy link

Roger that, that makes sense. I don't have much context context here so I appreciate the extra explanation :)

@XhmikosR
Copy link
Contributor Author

This was fixed by @nschonni's #4287

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

No branches or pull requests

4 participants