Skip to content

Conversation

@kishvanchee
Copy link
Contributor

@kishvanchee kishvanchee commented Jul 10, 2019

Hi Team,

I have added an example snippet for the str.isupper() method in the documentation.

This is my first contribution and pull request. Please let me know if I am doing it correctly. I am a bit unsure about whether I should have run any tests for making changes to the documentation.

EDIT: Just finished signing the CLA.

@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 we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Jul 10, 2019
Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

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

@kishvanchee Welcome and thanks for the contribution! With regards to the conventions used by the docs on code examples, there's a specific section in the devguide.

For the most part, examples are used rather sparingly in the official docs, for the purpose of breaking down a complex function. The module docs are to provide a detailed technical explanation, but not to explain the basics in depth. Something demonstrating the output of a simple function such as str.isupper() in detail would likely be better suited to the tutorial.

If you were to submit this to the tutorial, I would also recommend making the code a little bit more compact:

>>> 'BANANA'.isupper()
True
>>> 'banana'.isupper()
False
>>> 'baNana'.isupper()
False

@aeros
Copy link
Contributor

aeros commented Jul 11, 2019

Also, the docs don't require any tests to be provided with the PR, that's usually only for code related modifications, such as bug fixes and feature enhancements. If you're specifically interested in helping out with the documentation (or anything else for that matter), I would also highly recommend checking out the bug tracker. From there, you can specifically look for documentation related issues.

Alternatively, for general issues that are particularly suitable for newer contributors, you can look for easy issues. From the side bar under Summaries select Easy issues.

@kishvanchee
Copy link
Contributor Author

kishvanchee commented Jul 11, 2019

@aeros167 Hi thank you very much for your feedback. I have a couple of points I would like to make.

For the most part, examples are used rather sparingly in the official docs, for the purpose of breaking down a complex function.

Why does this not apply for simpler functions? Simple - I would say is relative. For someone new to python/programming, they would highly benefit from examples. I am under the impression that the modules should be able to describe the methods properly, and examples, even if for simple ones, are more visually appealing and easier to skim through and understand quickly, than reading through the technical description.

From the specific portion of the devguide that you have linked,

For instance, the str.rpartition() method is better demonstrated with an example splitting the domain from a URL than it would be with an example of removing the last word from a line of Monty Python dialog.

I see no example for str.rpartition() even though the doc says it would be better explained by using a certain example compared to using another example. I also feel this does not mean examples should not be used to explain the simpler methods.

From the link

Short code examples can be a useful adjunct to understanding. Readers can often grasp a simple example more quickly than they can digest a formal description in prose.

I still feel that examples would be better suited even to explain simpler functions purely because visually it is easier to read and understand than to read through the technical description.

@kishvanchee
Copy link
Contributor Author

kishvanchee commented Jul 11, 2019

To expand on my previous comment, str.strip() has an example snippet, and str.rstrip() also has an example snippet. Even though rstrip might be an intuitive extension of strip, an example snippet is still present.

@aeros
Copy link
Contributor

aeros commented Jul 11, 2019

No problem!

I also feel this does not mean examples should not be used to explain the simpler methods.

I didn't mean to imply that the devguide explicitly specified that simpler functions should not have examples, I was just linking to it as a introductory resource for providing code examples. The devguides provide an overview for contributing, but they rarely act as set-in-stone rules.

My earlier explanation of not explicitly detailing the more simple functions on the docs is more so my own feedback on this issue. If anything, since we now have a more detailed tutorial, it would potentially be worthwhile to consider reducing the total number of examples on the docs and moving some to the tutorial as appropriate. Particularly for the boolean is... functions. There's one line on the devguide which summarizes this approach:

More documentation is not necessarily better documentation. Err on the side of being succinct.

It's quite the difficult balance to maintain when determining whether or not a particular function should contain examples, especially as the size of the documentation continuously grows. For the difference between str.isupper() and str.strip(), my takeaway is that str.strip() provides significant additional context with the visual example of the input and output. Personally, I don't think that an example for str.isupper() would provide quite as much value.

Regardless of whether or not this example is considered suitable for the docs (the core devs will ultimately make that decision anyways), I would highly suggest condensing the example down to a few lines, perhaps even further than my earlier example. If you look at most of the other examples for functions with a boolean output, they mostly have one True example and one False example. bytearray.isalpha() provides an example of this.

@aeros
Copy link
Contributor

aeros commented Jul 11, 2019

Simple - I would say is relative

I do fully agree with your stance on "simple" being relative, perhaps that's not the best term to accurately describe what I was meaning. In retrospect, "complex" was not very accurate either. By simple, I more so meant being able to explain the implementation with very minimal technical details, and a function name that accurately implies the behavior.

Also, I just realized that there is an example already present for isupper(), over in the details of bytearray.isupper(). It's a bit further down on the page.

Perhaps it would make sense to provide a link to this section in the description of str.isupper(), but I think it would probably be a bit redundant to provide a separate example in both sections that effectively represent the same behavior.

Return true if all cased characters [4]_ in the string are uppercase and
there is at least one cased character, false otherwise.

>>> s = 'BANANA'
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your patch,
but could simplify the number of lines?

>>> 'BANANA'.isupper()
>>> True
>>> 'banana'.isupper()
>>> False
>>> 'baNana'.isupper()
>>> False
>>> ' '.isupper()
>>> False

Copy link
Contributor

Choose a reason for hiding this comment

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

@matrixise:

but could simplify the number of lines?

Based on bytearray.isupper(), I think if we are going to add this to the docs it could be reduced down to two lines. Also, the >>> prompt should only be present in the code input, not the output (based on the REPL behavior):

>>> 'BANANA'.isupper()
True
>>> 'baNana'.isupper()
False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi. I have fixed it according to your recommendation. Please have a look?

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@kishvanchee
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@matrixise: please review the changes made to this pull request.

Copy link
Member

@matrixise matrixise left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution

@matrixise matrixise merged commit 7183064 into python:master Sep 13, 2019
@kishvanchee kishvanchee deleted the dev_kish branch September 13, 2019 11:24
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 skip issue skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants