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: unify format of iterables #20036

Closed
wants to merge 1 commit into from
Closed

doc: unify format of iterables #20036

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Also, make signatures easier for copy-paste testing.

Also, make signatures easier for copy-paste testing.
@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 14, 2018
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 14, 2018
@vsemozhetbyt
Copy link
Contributor Author

@TimothyGu
Copy link
Member

I personally feel @@ is nicer to read, and especially compact in tables of contents. But I won't block this.

@vsemozhetbyt
Copy link
Contributor Author

@TimothyGu I mean mostly these reasons:

  • Code examples do not contain low-level fragments to copy-paste and test things like iterator.next(), so it can be handy to have JS code in signatures.
  • @ symbols just in these two headings make general doctools parsing RegExps more complicated.

However, if somebody does have objections, I will revert these parts.

@vsemozhetbyt
Copy link
Contributor Author

Landed in ff4ca64

@vsemozhetbyt vsemozhetbyt deleted the doc-iter branch April 15, 2018 08:37
vsemozhetbyt added a commit that referenced this pull request Apr 15, 2018
Also, make signatures easier for copy-paste testing.

PR-URL: #20036
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@TimothyGu
Copy link
Member

@vsemozhetbyt

  • Code examples do not contain low-level fragments to copy-paste and test things like iterator.next(), so it can be handy to have JS code in signatures.

Interesting point. On the other hand, it inspires the question of who would want to copy-paste this function in the first place since it's such a low level API.

  • @ symbols just in these two headings make general doctools parsing RegExps more complicated.

Fair point, but I'm not convinced that should be the primary reason this gets landed.


Procedurally, I am not very happy with how this PR was fast-tracked. Even though I did not explicitly block this pull request, I'd like to note that COLLABORATOR_GUIDE says:

Before landing pull requests, sufficient time should be left for input from other Collaborators.

In the future I would prefer to get a chance to reply to any responses to my comments before the PR gets merged rather than after, especially considering the fact that this PR was created and applied in a 20-hour span over the weekends.

@vsemozhetbyt
Copy link
Contributor Author

@TimothyGu Sorry. I will wait more time if any non-blocked concerns expressed.

jasnell pushed a commit that referenced this pull request Apr 16, 2018
Also, make signatures easier for copy-paste testing.

PR-URL: #20036
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
Also, make signatures easier for copy-paste testing.

PR-URL: nodejs#20036
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.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. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants