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

bpo-30174: remove duplicate definition from pickletools #1301

Merged
merged 2 commits into from
Apr 27, 2017

Conversation

JelleZijlstra
Copy link
Member

There's another almost identical definition of bytes1 above this one.

The only difference is a minor one in the docstring: the first one has
"the number of bytes in the string" and the second one has just "the
number of bytes". I like the first wording better so I kept it.

There's another almost identical definition of bytes1 above this one.

The only difference is a minor one in the docstring: the first one has
"the number of bytes in the string" and the second one has just "the
number of bytes". I like the first wording better so I kept it.
@mention-bot
Copy link

@JelleZijlstra, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tim-one, @mdickinson and @avassalotti to be potential reviewers.

@JelleZijlstra
Copy link
Member Author

I think this qualifies as "trivial" but I don't have the rights to add that label.

@berkerpeksag
Copy link
Member

Unfortunately, this is a code cleanup so we at least need to ping pickle maintainers on bugs.p.o. Sometimes there might be a valid or historical reason for this kind of code duplication.

@JelleZijlstra
Copy link
Member Author

OK, I'll open an issue.

@JelleZijlstra JelleZijlstra changed the title remove duplicate definition from pickletools bpo-30174: remove duplicate definition from pickletools Apr 26, 2017
@tim-one
Copy link
Member

tim-one commented Apr 26, 2017

I'm the original pickletools author, and the change looks good to me. The "bytes" type didn't exist at the time, so this code was added later. The current duplication obviously serves no purpose, and was certainly a mistake. I don't understand how the new git workflow works yet else I'd do something tangible to get this patch applied ;-)

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

The variant without " in the string" is more correct and consistent with bytes4 and bytes8.

@serhiy-storchaka serhiy-storchaka merged commit 5a4e3d8 into python:master Apr 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants