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

doc: Add blurb to N-API recommending the use of badges #22801

Closed
wants to merge 7 commits into from
Closed

doc: Add blurb to N-API recommending the use of badges #22801

wants to merge 7 commits into from

Conversation

ddlan
Copy link

@ddlan ddlan commented Sep 11, 2018

Add blurb to N-API documentation indicating that we recommend the use of badges
Fixes: #22796

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

Add blurb to N-API documentation indicating that we recommend the use of badges 
Fixes: #22796
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Sep 11, 2018
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
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.

I won't block this, but I don't think it belongs in the docs.

@mhdawson
Copy link
Member

@cjihrig just wondering if you have alternate suggestions where you think it would make more sense to publicize this kind of info?

@mhdawson
Copy link
Member

@gabrielschulhof FYI in case you have not seen this yet.

Made sure text has a maximum of 80 characters per line
Added space after heading
Deleted extra line at end of blurb
@ddlan ddlan closed this Sep 13, 2018
@vsemozhetbyt
Copy link
Contributor

@ddlan Did you close it intentionally?

@ddlan
Copy link
Author

ddlan commented Sep 13, 2018

@vsemozhetbyt Yes, would it be better for me to leave it open?

@vsemozhetbyt
Copy link
Contributor

Well, I see two approvals and no blocking objections, so this can be landed if I am not mistaken)

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2018

just wondering if you have alternate suggestions where you think it would make more sense to publicize this kind of info?

Sorry, I missed that response. I don't have an alternative suggestion, I just don't feel like it belongs in the official Node docs. Shields.io and other services offer a bunch of different badges, but we don't suggest using any other ones. And, why do I need a badge? Why not just write it out in the project's README. Again, I don't feel strongly enough to block it, but it seems odd.

@ddlan ddlan reopened this Sep 13, 2018
@gabrielschulhof
Copy link
Contributor

@cjihrig we're taking this step to drive adoption of N-API. We do so after having received feedback in support in nodejs/admin#221.

@gabrielschulhof
Copy link
Contributor

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Please don’t include external resources in our official API docs, since it leaks usage data to the image hoster.

That being said, I agree with @cjihrig in that this is not really something that should go into our official documentation.

@ddlan
Copy link
Author

ddlan commented Sep 16, 2018

@addaleax In that case, what's the best way to add the images to the documentation?

@addaleax
Copy link
Member

I suppose that if we do add this, downloading them to doc/api_assets and including them from assets/ would be the best way?

The badge images were originally hosted on an external website but has been added to the node repository as not to leak usage data.
@danbev
Copy link
Contributor

danbev commented Sep 20, 2018

@addaleax With the latest commit, would you be alright with landing this?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@danbev I don’t know … it still seems out-of-place in our Node.js core documentation. This is API/reference docs, not a guide for repository/package management.

doc/api/n-api.md Outdated
![N-API v1 Badge](../api_assets/N-API%20v1%20Badge.svg)
![N-API v2 Badge](../api_assets/N-API%20v2%20Badge.svg)
![N-API v3 Badge](../api_assets/N-API%20v3%20Badge.svg)
![N-API Experimental Version Badge](../api_assets/N-API%20Experimental%20Version%20Badge.svg)
Copy link
Member

Choose a reason for hiding this comment

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

Have you verified that this works in the make doc output? I think most of our assets are included via assets/, not some ../-prefixed path

@gabrielschulhof
Copy link
Contributor

@ddlan could you please redo this PR against the doc branch at abi-stable-node, and add the blurb to README.md in that branch instead? That might be a better place for it.

@Trott
Copy link
Member

Trott commented Nov 16, 2018

It seems like this has not reached consensus and conversation has stalled. We should either close it or escalate to the TSC for a decision. I'm going to close it, but feel free to leave a comment if you feel that escalation to the TSC is the better approach. (I'm personally of the opinion that this is somewhat out of place in our docs and is better suited for another repo, as suggested by @gabrielschulhof.)

@Trott Trott closed this Nov 16, 2018
@Trott
Copy link
Member

Trott commented Nov 16, 2018

@ddlan Thanks for the pull request. I hope the experience was not discouraging and I hope you will continue to submit pull requests for Node.js!

gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this pull request Jan 9, 2019
Add blurb to N-API documentation indicating that we recommend the use
of badges.

Fixes: nodejs/node#22796
Re: nodejs/node#22801
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this pull request Jan 9, 2019
Add blurb to N-API documentation indicating that we recommend the use
of badges.

Fixes: nodejs/node#22796
Fixes: nodejs#333
Re: nodejs/node#22801
gabrielschulhof pushed a commit to nodejs/abi-stable-node that referenced this pull request Jan 19, 2019
Add blurb to N-API documentation indicating that we recommend the use
of badges.

PR-URL: #357
Fixes: nodejs/node#22796
Fixes: #333
Re: nodejs/node#22801
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add blurb to N-API documentation indicating that we recommend the use of badges