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

[linting] Use eslint to lint code. + minor refactoring + Travis CI setup #1020

Merged
merged 31 commits into from
Feb 25, 2018

Conversation

Feder1co5oave
Copy link
Contributor

@Feder1co5oave Feder1co5oave commented Jan 20, 2018

See the previous discussion at #999.

closes #1014 and closes #1028 .
closes #967

See the commits list for the history of edits.

Apart from some unnoticeable edits, I've changed some minor code style (see var declarations) and refactored the replace() function in a OOP way. I believe it looks neat and self-documenting, I'd like to hear from others on this, and accept different proposals.

With this PR, eslint is completely happy with the source code as it is, and reports no warning or errors.
The programmer can run npm run lint to autofix code style, and get linting feedback before submitting new PRs.

Future steps: linting test/index.js 😄

Thanks a lot to @smhg and @joshbruce for their contributions.

@Feder1co5oave Feder1co5oave changed the title [Proposal] Use eslint to lint code. + minor refactoring [linting] Use eslint to lint code. + minor refactoring Jan 20, 2018
@joshbruce joshbruce added this to the 0.5.0 - Architecture and extensibility milestone Jan 20, 2018
@joshbruce
Copy link
Member

I like it! Definitely seems easier to read and makes it more declarative in its intent.

I'm going to flag it for 0.5.0 for now to allow for conversation while still focusing on fixing known issues...does anyone think it should be part of 0.4.0?? (Do we think it will actually help folks fix the defects we've found to date??)

@UziTech
Copy link
Member

UziTech commented Feb 2, 2018

I think this would help development greatly.

Since this doesn't affect dependents, I see no reason to wait for a release.

@Feder1co5oave I am getting a warning that eslint-plugin-import and eslint-plugin-promise are peer dependencies of eslint-config-standard. Should those be added under devDependencies?

@joshbruce
Copy link
Member

@UziTech and @Feder1co5oave: Let me know if I should merge now or wait until the devDependencies thing is figured out. Sorry if I'm out of sync - long day of humaning. :)

@Feder1co5oave
Copy link
Contributor Author

Let's wait a bit, I want to do a dependency check-up beforehand, and be sure to get dependency versions correct.

@Feder1co5oave Feder1co5oave mentioned this pull request Feb 2, 2018
7 tasks
@Feder1co5oave
Copy link
Contributor Author

Feder1co5oave commented Feb 2, 2018

Can you confirm that using local copies of gulp and eslint is the right way to go? I think it's safe assuming the dependencies are installed locally?
NEVERMIND, I found this. Turns out npm puts node_modules/.bin/ in $PATH when running tasks.

@smhg
Copy link
Contributor

smhg commented Feb 2, 2018

You're absolutely right. Local copies are definitely preferred.
This comment was confusing. When you want to run the ESLint --init script once when only having a local copy, you'll need to add the full node_modules path. But for reusable scripts NPM is great (with its path, among others).

@Feder1co5oave
Copy link
Contributor Author

Thanks for confirming that. I forgot to push.

@UziTech
Copy link
Member

UziTech commented Feb 19, 2018

prepack sounds like the right place to put it. That way if someone requires marked as a git url the build will happen before it is downloaded.

@joshbruce
Copy link
Member

@UziTech: Agreed. Going to submit a WIP PR to continue conversation there - also trying to learn all the things!

@Feder1co5oave Feder1co5oave changed the title [linting] Use eslint to lint code. + minor refactoring [linting] Use eslint to lint code. + minor refactoring + Travis CI setup Feb 22, 2018
@Feder1co5oave Feder1co5oave added the RR - refactor & re-engineer Results in an improvement to developers using Marked, or end-users, or both. label Feb 23, 2018
Copy link
Member

@joshbruce joshbruce left a comment

Choose a reason for hiding this comment

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

This all looks good to me. Would like to get a second pair of eyes though as it looks like we have lint + Travis (woohoo! on both). Want to make sure some of the changes to marked.js are just making things a bit more explicit; the addition/change to edit().replace, for example.

@joshbruce
Copy link
Member

Looks like @UziTech will be coming in behind me; so, let me know if this should result in an NPM-publish once merged. Would like to go through that process with CI and whatnot a couple more times before adding more publishers to refine the PR templates and process docs.

@joshbruce
Copy link
Member

Thank you @Feder1co5oave. You're awesome!

- "0.8"
- "0.6"
- "4"
- "lts/*"
Copy link
Member

Choose a reason for hiding this comment

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

we should probably add node to the versions to get the latest stable version (Node 9.x right now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. I wanna leave the lts in there too since it is required by the selenium-webdriver which is something I'm working with to do cross-browser tests on Browserstack 😉

Copy link
Member

Choose a reason for hiding this comment

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

Ya the minimum I always test is lts/* and node

@Feder1co5oave
Copy link
Contributor Author

https://travis-ci.org/Feder1co5oave/marktex/builds/345826064

(still failing due to that link encoding issue I'm solving)

@Feder1co5oave Feder1co5oave merged commit 56d1bcf into markedjs:master Feb 25, 2018
@Feder1co5oave
Copy link
Contributor Author

Josh I think you can finally enable Travis on this repo 🥂

@joshbruce
Copy link
Member

See #1076

@Feder1co5oave Feder1co5oave deleted the lint branch March 3, 2018 17:44
@TheDancingCode
Copy link

Is it the aim to eventually remove the rules that override the standard config? I can help with that if you want.

@styfle
Copy link
Member

styfle commented Mar 4, 2018

@TheDancingCode

I don’t believe so. I think the idea was to get lint tests that matched the current code style. But we need to figure out our roadmap a little better.

@joshbruce
Copy link
Member

@TheDancingCode: To be honest, I'm not sure I'm following (sorry, still figuring out the vernacular of the users). Would you at least be willing to submit an issue with a little exposition on the idea??

@Feder1co5oave
Copy link
Contributor Author

Feder1co5oave commented Mar 4, 2018

Yes, the original idea was to progressively adapt the code style to (semi)standard.
However, the only rules I would possibly re-enable are

@TheDancingCode
The following rules are disabled because they would not help readability:

  • no-cond-assign
  • no-useless-escape
  • no-return-assign

The following must stay disabled:

  • no-control-regex

I don't think it worth working on this (apart the one-var). Consider that re-linting code may involve big patches that will likely bring merge conflicts with PRs that were branched from an earlier commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RR - refactor & re-engineer Results in an improvement to developers using Marked, or end-users, or both.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants