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

Clean up tests #1482

Merged
merged 22 commits into from
May 6, 2019
Merged

Clean up tests #1482

merged 22 commits into from
May 6, 2019

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Apr 25, 2019

Marked version: master

Description

  • Move all tests to jasmine (still .md and .html files for original and new)
  • Remove old gfm_ and cm_ tests from new
  • Bench uses CommonMark specs
  • Create npm run test:update to update CommonMark and GFM specs
  • Remove standard from eslint config for marked.js to disallow const and template strings for older versions of node and browsers
  • Remove browser test that wasn't working anyways

Contributor

  • Test(s) exist to ensure functionality and minimize regression

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@UziTech UziTech mentioned this pull request Apr 25, 2019
6 tasks
bin/marked Outdated
@@ -13,7 +13,7 @@ var fs = require('fs'),
* Man Page
*/

function help() {
function help () {
Copy link
Member

Choose a reason for hiding this comment

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

But why spaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured it should be consistant and since we are using standard might as well use their recommendation.

I personally don't care one way or the other but they were inconsistent before.

Copy link
Member

Choose a reason for hiding this comment

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

I think it should have the fewest changes possible (preferably none) to lib/marked.js and bin/marked

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the philosophical question "why?" I think the argument is that is makes visual grepping easier.

help() = calling a help function
help () = defining a help function

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I did pick the least changes. Making it consistent would have changed it either way.

Copy link
Member

Choose a reason for hiding this comment

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

My brain takes issue with this. Which standard/style/recommendation is this from (sorry, might have missed a mention somewhere)?

I haven't seen a language or style that adds a space between the name and the argument list container. Mainly just curious because it seems like it might add confusion not reduce it.

Copy link
Member

Choose a reason for hiding this comment

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

I think I did pick the least changes

This is changing every single function. This is not necessary. We should make the tools follow the standards of the project, not conform to someone else's standard (unless there is clear value added).

Copy link
Member Author

Choose a reason for hiding this comment

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

standard js (which we have been using with eslint) has space-before-function-paren eslint rule set to "always". We had that rule turned off which made our code inconsistant.

This PR removes standard from eslint since it doesn't allow us to catch es6 syntax in eslint but I added all of standard's rules to keep it consistant.

I changed it to never allow a space between a function and parens

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the link.

@UziTech
Copy link
Member Author

UziTech commented Apr 27, 2019

eslint/eslint#11658 is the reason I removed "extends": "standard" from eslint

@UziTech
Copy link
Member Author

UziTech commented May 3, 2019

@joshbruce this should be ready to merge

@joshbruce
Copy link
Member

@UziTech - Confilicts. Sorry - probably should have merged this first.

@UziTech
Copy link
Member Author

UziTech commented May 4, 2019

@joshbruce I fixed the conflicts

@joshbruce joshbruce merged commit 40ca559 into markedjs:master May 6, 2019
@UziTech UziTech deleted the branch-json branch May 6, 2019 14:50
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants