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

doc: documented make docopen #19321

Closed
wants to merge 7 commits into from
Closed

doc: documented make docopen #19321

wants to merge 7 commits into from

Conversation

AyushG3112
Copy link
Contributor

@AyushG3112 AyushG3112 commented Mar 13, 2018 •
edited
Loading

Documented make docopen as a way to read documentation in the browser.

Checklist

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Mar 13, 2018
@addaleax
Copy link
Member

Fwiw, you don’t need a webserver to read the docs, just opening the generated HTML files in your browser works.

@AyushG3112
Copy link
Contributor Author

@addaleax true, and perhaps I should have been more explicit in the description.

But personally as a user, I still prefer the SimpleHTTPServer way because if I close my browser, I do not have to navigate the HTML files to open the docs again, but that's just me.

@addaleax
Copy link
Member

@AyushG3112 Yeah, not telling you what to do :) It might be nice to document make docopen too though, it does basically this.

@AyushG3112
Copy link
Contributor Author

@addaleax oh, I did not know about make docopen.

Should I document it in this PR, because if I do the title of the PR would not be enough?
Also, If I do document make docopen, should I add it in addition to the SimpleHTTPServer or replace the SimpleHTTPServer with it?

@addaleax
Copy link
Member

@AyushG3112 All of that is completely up to you. :)

Should I document it in this PR, because if I do the title of the PR would not be enough?

You can always change a PR title, and if you use git commit --amend (and git push --force-with-lease instead of git push), you can also change the commit message & contents of an existing commit.

Let us know if you need any help!

@AyushG3112 AyushG3112 changed the title doc: Added python SimpleHTTPServer as a method reading docs in browser doc: documented make doconly Mar 13, 2018
@AyushG3112
Copy link
Contributor Author

AyushG3112 commented Mar 13, 2018 •
edited
Loading

@addaleax Changed :) . Could you let me know if any improvement is needed? First time doc-only contribution to Node.JS here

@addaleax
Copy link
Member

This seems perfectly fine to me. :)

Light CI: https://ci.nodejs.org/job/node-test-pull-request-lite/256/

@vsemozhetbyt
Copy link
Contributor

Sorry, I cannot find any doconly task in Makefile. Should it be docopen?

node/Makefile

Lines 672 to 674 in 52e869b

.PHONY: docopen
docopen: $(apidocs_html)
@$(PYTHON) -mwebbrowser file://$(PWD)/out/doc/api/all.html

@AyushG3112 AyushG3112 changed the title doc: documented make doconly doc: documented make docopen Mar 13, 2018
@vsemozhetbyt
Copy link
Contributor

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/257/

@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 13, 2018
Trott
Trott reviewed Mar 14, 2018
View reviewed changes
BUILDING.md Outdated
@@ -206,6 +206,15 @@ To read the documentation:
$ man doc/node.1
```

If you prefer to read the documentation in the browser,
Copy link
Member

Choose a reason for hiding this comment

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

Tiny optional nit: in the browser -> in a browser

Trott
Trott reviewed Mar 14, 2018
View reviewed changes
BUILDING.md Outdated
$ make docopen
```

This will open up a browser tab/window with the documentation.
Copy link
Member

Choose a reason for hiding this comment

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

Optional nit: open up a browser tab/window -> open a browser

Trott
Trott reviewed Mar 14, 2018
View reviewed changes
BUILDING.md Outdated
@@ -206,6 +206,15 @@ To read the documentation:
$ man doc/node.1
```

If you prefer to read the documentation in the browser,
run the following once `make doc` is done:
Copy link
Member

Choose a reason for hiding this comment

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

Optional nits: once -> after and done -> finished

@AyushG3112
Copy link
Contributor Author

@Trott updated. Also, I rebased to master before pushing, which is the reason for increased number of changes.

@AyushG3112
Copy link
Contributor Author

@addaleax can this land?

trivikr
trivikr approved these changes Mar 26, 2018
View reviewed changes
Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

Confirmed that comments of @Trott were incorporated and make docopen works as described.
This PR is good to land.

@trivikr
Copy link
Member

trivikr commented Mar 26, 2018

CI is failing because of merge conflict.
CI: https://ci.nodejs.org/job/node-test-pull-request-lite/336/
Console output: https://ci.nodejs.org/job/node-test-commit-lite/492/console

@AyushG3112 Please rebase your changes as defined in this documentation https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#step-9-discuss-and-update

@AyushG3112
doc: Added python SimpleHTTPServer as a method reading docs in browser
2d8e7de 
Documented an easier way for users to browse documentation without
setting up a whole web environment. It make the process of reading the
latest documentation, and comparing it with the documentation of
older versions on the Node.JS website a lot easier.
@AyushG3112
doc: document make doconly
ecb58d0 
Documented `make doconly` as a way to read documentation in the browser.
@AyushG3112
doc: fix doconly to docopen
c355442 
PR-URL: nodejs#19321
@AyushG3112
doc: Added python SimpleHTTPServer as a method reading docs in browser
0770c1f 
Documented an easier way for users to browse documentation without
setting up a whole web environment. It make the process of reading the
latest documentation, and comparing it with the documentation of
older versions on the Node.JS website a lot easier.
@AyushG3112
doc: document make doconly
abd7fc0 
Documented `make doconly` as a way to read documentation in the browser.
@AyushG3112
doc: fix doconly to docopen
b971a1e 
PR-URL: nodejs#19321
@AyushG3112
doc: address nits requested by @Trott
f6a70dc 
PR-URL: nodejs#19321
@AyushG3112 AyushG3112 force-pushed the building-docs-python-simplehttpserver branch from 2afa7b1 to f6a70dc Compare March 26, 2018 04:50
@AyushG3112
Copy link
Contributor Author

@trivikr Rebased.

@trivikr
Copy link
Member

trivikr commented Mar 26, 2018

New CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/337/

@trivikr
Copy link
Member

trivikr commented Mar 26, 2018

Rerun of CI-lite was successful https://ci.nodejs.org/job/node-test-pull-request-lite/338/

@trivikr
Copy link
Member

trivikr commented Mar 26, 2018

Landed in 0d5720b

@trivikr trivikr closed this Mar 26, 2018
trivikr pushed a commit that referenced this pull request Mar 26, 2018
@AyushG3112 @trivikr
doc: document make docopen
0d5720b 
Documented `make docopen` as a way to read documentation in the browser.

PR-URL: #19321
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request Mar 27, 2018
@AyushG3112 @targos
doc: document make docopen
5e9f929 
Documented `make docopen` as a way to read documentation in the browser.

PR-URL: #19321
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 4, 2018
@AyushG3112 @BethGriggs
doc: document make docopen
c3ecf05 
Documented `make docopen` as a way to read documentation in the browser.

PR-URL: #19321
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
Merged
This was referenced Dec 18, 2018
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@trivikr trivikr trivikr approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@hiroppy hiroppy hiroppy approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@AyushG3112 @addaleax @vsemozhetbyt @trivikr @jasnell @Trott @lpinca @hiroppy @nodejs-github-bot