bpo-36322: Fix parameter name at dbm.gnu.open docs by rougeth · Pull Request #12095 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Hi @rougeth, thanks for taking the time to improve the documentation!
I think dbm.ndbm.open suffers from the same issue, can you fix it too?
Updated, thanks for reviewing @remilapeyre. This flags options is quite confusing, in ndbm it accepts only one flag at the time, doesn't make sense to be in plural.
Sorry of the force-pushed commits, wasn't getting the git name/email correctly configured.
You are right, the good thing is that it does not take keyword arguments:
>>> dbm.ndbm.open(filename='foo', flags='r', mode=438)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: open() takes no keyword arguments
>>>
so we can change the name of the argument without breaking current code.
I think we should fix the docstring for dbm.ndbm.open rather than changing the doc here.
@remilapeyre the issue is that in every other reference I could find about ndbm uses flags instead of flag, so I'm not sure about changing the interface/docs.
Were those references in the documentation? Since there is no way to use keyword arguments with dbm.ndbm.open it would not change the interface.
No, not in Python documentation. What I meant was that our documentation would be different from every other place (considering a fast and superficial web search) that describes how to use ndbm.open.
If only the docstring is changed, there will be inconsistency between it and how the function was actually implement, is that something ok?
Either way, I think this goes a bit further than "a trivial change". I'm going to open an issue on bpo so that we can continue from there and I think this PR can go as it is. What do you think?
Hi @rougeth, you are right that this goes further than the initial change and opening a issue on bpo so people more knowledgeable can give the correct way forward is always a good idea.
Please do open a new bug report; if you want, you can rollback the change made on dbm.ndbm.open on this PR (really sorry about that) so we can get this documentation improvement merged and we will open a new PR with whatever the consensus on dbm.ndbm.open will be.
@remilapeyre nothing to be sorry about :) But I still think the last commit is valid tho. I'd prefer to go with the way it's now and if it's needed, we can change it again. Thanks for the help!
Fix parameter name at dbm.gnu.open docs bpo-36322: Fix parameter name at dbm.gnu.open docs
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you're not only right about the incorrect name, but it also seems default values are missing.
| @@ -149,12 +149,12 @@ supported. |
|---|
| raised for general mapping errors like specifying an incorrect key. |
| .. function:: open(filename[, flag[, mode]]) |
| .. function:: open(filename[, flags[, mode]]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @@ -264,12 +264,12 @@ to locate the appropriate header file to simplify building this module. |
|---|
| Name of the ``ndbm`` implementation library used. |
| .. function:: open(filename[, flag[, mode]]) |
| .. function:: open(filename[, flags[, mode]]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Closing as the parameters under question are positional-only and thus their names don't matter.
