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.

Sign up for GitHub

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

Jump to bottom

test: conver html table in README.md to markdown table #14291

Closed
wants to merge 0 commits into from
Closed

test: conver html table in README.md to markdown table #14291

wants to merge 0 commits into from

Conversation

vixony
Copy link

@vixony vixony commented Jul 16, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 16, 2017
@vixony
Copy link
Author

vixony commented Jul 16, 2017

commit my first doc change

@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jul 16, 2017
Trott
Trott approved these changes Jul 16, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. I think it's important for us to keep our README files comprehensible when viewed as plain text as much as we can. Thanks for the contribution!

vsemozhetbyt
vsemozhetbyt reviewed Jul 16, 2017
View reviewed changes
test/README.md Outdated
|abort |No | Tests for when the ``` --abort-on-uncaught-exception ``` flag is used.|
|addons |Yes | Tests for [addon](https://nodejs.org/api/addons.html) functionality along with some tests that require an addon to function properly.|
|cctest |Yes | C++ test that is run as part of the build process.|
|common | | Common modules shared among many tests. [[Documentation]](./common/README.md |
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: error in the link format.

Copy link
Member

Choose a reason for hiding this comment

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

Let's make sure this gets fixed before we land. (Or whoever lands it can fix it)

cjihrig
cjihrig approved these changes Jul 16, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with nit addressed.

tniessen
tniessen approved these changes Jul 16, 2017
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

LGTM with #14291 (comment) addressed

@tniessen tniessen self-assigned this Jul 16, 2017
tniessen
tniessen requested changes Jul 21, 2017
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are some inconsistencies apart from #14291 (comment):

  1. Why do some lines end with a | while others don't?
  2. You sometimes put a space before | and sometimes not.
  3. Some lines contain unnecessary whitespace between words (e.g. here).

Also, the line lengths have become quite ugly, but I guess there is no way around when working with MD tables.

@mscdex
Copy link
Contributor

mscdex commented Jul 21, 2017

I agree with @tniessen.

@gibfahn
Copy link
Member

gibfahn commented Jul 22, 2017

Also, the line lengths have become quite ugly, but I guess there is no way around when working with MD tables.

Agreed, but I think the html table looks worse.

@Trott Trott closed this Jul 25, 2017
Trott pushed a commit to Trott/io.js that referenced this pull request Jul 25, 2017
@sealkingw @Trott
test: convert table in test doc to markdown table
85939bd 
PR-URL: nodejs#14291
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@Trott
Copy link
Member

Trott commented Jul 25, 2017 •
edited
Loading

I fixed up the nits and landed in bb11eb5af892984c76ac5ba7580887ae24396d84.

Thanks for the contribution! 🎉

@Trott
Copy link
Member

Trott commented Jul 25, 2017

Let's try again...

I fixed up the nits and landed in 85939bd.

Thanks for the contribution! 🎉

addaleax pushed a commit that referenced this pull request Jul 27, 2017
@sealkingw @addaleax
test: convert table in test doc to markdown table
cd0fffd 
PR-URL: #14291
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@addaleax addaleax mentioned this pull request Aug 2, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr left review comments

@tniessen tniessen tniessen requested changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott approved these changes

Assignees

@tniessen tniessen

Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.