Skip to content

bpo-26506: hex() documentation: mention "%x" % int (Original patch by Manvi B) #2479

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

Closed
wants to merge 6 commits into from

Conversation

sharanry
Copy link

@sharanry sharanry commented Jun 28, 2017

Refs:
http://bugs.python.org/issue26506

Original patch by Manvi B

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@sharanry sharanry changed the title bpo-26506: hex() documentation: mention "%x" % int bpo-26506: hex() documentation: mention "%x" % int (Patch by Manvi B) Jun 28, 2017
@sharanry sharanry changed the title bpo-26506: hex() documentation: mention "%x" % int (Patch by Manvi B) bpo-26506: hex() documentation: mention "%x" % int (Original patch by Manvi B) Jun 28, 2017
@Mariatta Mariatta added needs backport to 3.6 docs Documentation in the Doc dir labels Jun 28, 2017
also :func:`format` for more information.

>>> num = 14
>>> format(num, '#b'), format(num, 'b')
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to remove the num variable and just write 14 directly, to simplify examples (and easier to copy/paste).

'-0b1010'

If prefix "0b" is desired or not, you can use either of the following ways. See
also :func:`format` for more information.
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to move the "See also..." sentence after the examples.

method that returns an integer.
If you want to convert an integer number to uppercase or lowercase hexadecimal
string either with prefix or not, you can use either of the following ways. See
also :func:`format` for more information.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, move "See also ..." above.

string either with prefix or not, you can use either of the following ways. See
also :func:`format` for more information.

>>> num=255
Copy link
Member

Choose a reason for hiding this comment

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

Ditto: replace num by its value (and remove num).

@vstinner
Copy link
Member

"Original patch by Manvi B", hum, please put this string in the commit message (use "git commit --amend" for example to edit the commit message), and include the email: [email protected].

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

(Please make suggested changes.)

@Mariatta
Copy link
Member

Regarding the commit message, the core developer who merge this PR can manually type it in after they press the "Squash and merge" button :)

Update functions.rst based on recommendations from haypo.

If prefix "0b" is desired or not, you can use either of the following ways.
See also :
func:`format` for more information.
Copy link
Member

Choose a reason for hiding this comment

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

This line is strange... I suggest to put "See also ..." sentence after the following ">>> ..." examples.

@sharanry
Copy link
Author

@Haypo I have made the changes you asked. Please review them.

>>> f'{14:#b}', f'{14:b}'
('0b1110', '1110')

See also :
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why :func:format is splitted in two lines. I expect an error when building the doc. Please don't split this statement into two lines.

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand what you are saying. Could you please be more specific?

Copy link
Member

Choose a reason for hiding this comment

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

You have ":" on line 102 and "func:format" on line 103. The documentation uses the Sphinx format. I don't think that your syntax is valid. Please write ":func:format" on the same line.

Copy link
Author

Choose a reason for hiding this comment

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

Never mind i understood what you are saying. Thanks!

string either with prefix or not, you can use either of the following ways.
See also :

func:`format` for more information.
Copy link
Member

Choose a reason for hiding this comment

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

Please move "See also .." after the example.


If you want to convert an integer number to octal string either with prefix "0o"
or not, you can use either of the following ways. See also :func:`format` for
more information.
Copy link
Member

Choose a reason for hiding this comment

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

Same here. I already asked you to do that :-(

@vstinner
Copy link
Member

@Mariatta: Would you mind to take a look to confirm that the doc change is fine. I don't know if we can "merge" the two "See also ..." sentence of the hex() doc. I don't know if t's worth it. Maybe just put them in the same paragraph?

@kushaldas
Copy link
Member

I think we should ask the original author if she wants to submit the PR.

@manvi77
Copy link
Contributor

manvi77 commented Jun 29, 2017

I am the original contributor. I will look into it again and can submit the PR.

@Mariatta
Copy link
Member

I think we should ask the original author if she wants to submit the PR.

I agree. I've just updated the Dev guide with guidance and protocol for converting the patches to PR:
https://devguide.python.org/pullrequest/#converting-an-existing-patch-from-the-b-p-o-to-github

I am the original contributor. I will look into it again and can submit the PR.

Please do.


See also :func:`format` for more information.

See also :func:`int` for converting a hexadecimal string to an integer using a base of 16.
Copy link
Member

Choose a reason for hiding this comment

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

Hexadecimal and base of 16 seems redundant

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is an integer using base of 16, for eg., int('0xff', 16). If it is redundant, I can remove it.


>>> hex(255)
'0xff'
>>> hex(-42)
'-0x2a'

If x is not a Python :class:`int` object, it has to define an __index__()
method that returns an integer.
If you want to convert an integer number to uppercase or lowercase hexadecimal
Copy link
Member

Choose a reason for hiding this comment

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

. . . to an uppercase or lowercase . . .

@Mariatta
Copy link
Member

Mariatta commented Jul 6, 2017

Thanks for the PR @sharanry
However, I accepted PR #2525 from the original author. So I will close this.
I hope you'll continue working on another issue in the bug tracker and submit a different PR.
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants