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: replace 'this platform' with the actual platform in the scalar type documentation #18085

Merged

Conversation

rpolley
Copy link
Contributor

@rpolley rpolley commented Dec 28, 2020

This pull changes the wording of the scalar type documentation so that it uses information from python's platform module (specifically {platform.system()} ({platform.machine()}) instead of the phrase 'this platform', which does not make it obvious that 'this platform' refers to the platform the documentation was built on, as well as which platform that is.

in the scalar documentation it's unclear that 'this platform' refers to
the platform that the doc build job ran on, so replace it with the name
of the platform using python's platform module
@eric-wieser
Copy link
Member

Remember these docstrings have to also make sense for users of help, where "this platform" is probably more clear.

Some other options:

  • Do both
  • Add a flag somewhere to omit "this platform" from only sphinx builds

def add_newdoc_for_scalar_type(obj, fixed_aliases, doc):
# note: `:field: value` is rST syntax which renders as field lists.
o = getattr(_numerictypes, obj)

character_code = dtype(o).char
canonical_name_doc = "" if obj == o.__name__ else ":Canonical name: `numpy.{}`\n ".format(obj)
alias_doc = ''.join(":Alias: `numpy.{}`\n ".format(alias) for alias in fixed_aliases)
alias_doc += ''.join(":Alias on this platform: `numpy.{}`: {}.\n ".format(alias, doc)
alias_doc += ''.join(":Alias on this platform ({} {}): `numpy.{}`: {}.\n ".format(platform.system(), platform.machine(), alias, doc)
Copy link
Member

Choose a reason for hiding this comment

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

Is system + machine better or worse than platform()?

(I have no idea)

Copy link
Contributor Author

@rpolley rpolley Jan 4, 2021

Choose a reason for hiding this comment

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

From what I can tell platform.system is pretty much the same as sys.platform. I threw in the platform.machine because I would expect what numeric types alias to each other would vary on the same OS between say, the 64 bit and 32 bit version. For example on my machine system + machine is Linux x86_64, and 'platformisLinux`.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, by platform I meant platform.platform()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

platform.platform has a ton of information (Linux-4.19.128-microsoft-standard-x86_64-with-glibc2.29 on my WSL2 setup), to the point where I think it would probably clutter up the documentation more than it would help if it were included more than once. Maybe have the full platform be somewhere on that page and then link back to it whenever 'this platform' is mentioned?

Copy link
Member

Choose a reason for hiding this comment

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

agreed, this looks good as is.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM, merging. Thanks @rpolley

def add_newdoc_for_scalar_type(obj, fixed_aliases, doc):
# note: `:field: value` is rST syntax which renders as field lists.
o = getattr(_numerictypes, obj)

character_code = dtype(o).char
canonical_name_doc = "" if obj == o.__name__ else ":Canonical name: `numpy.{}`\n ".format(obj)
alias_doc = ''.join(":Alias: `numpy.{}`\n ".format(alias) for alias in fixed_aliases)
alias_doc += ''.join(":Alias on this platform: `numpy.{}`: {}.\n ".format(alias, doc)
alias_doc += ''.join(":Alias on this platform ({} {}): `numpy.{}`: {}.\n ".format(platform.system(), platform.machine(), alias, doc)
Copy link
Member

Choose a reason for hiding this comment

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

agreed, this looks good as is.

@rgommers rgommers merged commit 48808e1 into numpy:master Jan 30, 2021
@rgommers rgommers added this to the 1.21.0 release milestone Jan 30, 2021
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

3 participants