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/remove spaces in snippets and re-format here and there #17976

Merged
merged 2 commits into from
Dec 16, 2020
Merged

DOC: Add/remove spaces in snippets and re-format here and there #17976

merged 2 commits into from
Dec 16, 2020

Conversation

rinarakaki
Copy link
Contributor

No description provided.

Comment on lines +196 to +198
>>> np.empty((2, 3)) # uninitialized, the result may vary
array([[3.73603959e-262, 6.02658058e-154, 6.55490914e-260],
[5.30498948e-313, 3.14673309e-307, 1.00000000e+000]])
Copy link
Member

Choose a reason for hiding this comment

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

"may vary" is a special marker understood by our CI tools, I think you might need to put it back where it was (if CI fails).

Copy link
Member

Choose a reason for hiding this comment

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

Interesting that this did not fail.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

potential fix in gh-18008

@@ -280,8 +280,8 @@ central part of the array and only prints the corners::

>>> print(np.arange(10000))
[ 0 1 2 ... 9997 9998 9999]
>>>
>>> print(np.arange(10000).reshape(100,100))
>>>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>>
>>>

otherwise someone's editor will just trim this again.

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

This largely looks great, thanks!

My only objections would be subjective, that:

  • Putting a trailing space after >>> isn't helpful, it will probably be stripped in a later commit by accident
  • Spaces around b**2 as b ** 2 etc seems unusual
  • I think some of the column alignment of comments was deliberate, but I don't feel strongly

>>> c
array([20, 29, 38, 47])
>>> b**2
>>> b ** 2
Copy link
Member

Choose a reason for hiding this comment

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

I'd be inclined to avoid spaces around **

@BvB93 BvB93 changed the title Add/remove spaces in snippets and re-format here and there DOC: Add/remove spaces in snippets and re-format here and there Dec 11, 2020
@charris
Copy link
Member

charris commented Dec 15, 2020

@rnarkk Could you make the suggested fixes?

@mattip mattip merged commit 8e038f4 into numpy:master Dec 16, 2020
@mattip
Copy link
Member

mattip commented Dec 16, 2020

Thanks @rnarkk

@rinarakaki
Copy link
Contributor Author

HI,

Thank you so much for providing helpful reviews.
Besides the commit I newly added, I would leave memo on the reviews:

  • The special marker '# may very' seemed not to be needed as all the CI's checks have passed.
  • I added a space every after '>>>' for Sphinx to recognise them as prompts. The concern - someone's editor would remove them by chance - should be cared by man/machine's review or finding Sphinx's settings.
  • Basically I put 2 spaces between code and comments but some comments are vertically aligned so that they look a comparison. Yet I'm not sure if I've done it all correctly nor there was sort of 'inter-snippet' alignment. If someone would tell me the basic rules, I'll follow them next time I submit PR.

Thank you!

@rinarakaki rinarakaki deleted the format-quickstart branch December 16, 2020 09:12
@eric-wieser
Copy link
Member

CI only passed because for some reason circle-ci did not run at all on this PR.

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.

None yet

4 participants