GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-29534: _decimal difference with _pydecimal by andrewnester · Pull Request #65 · python/cpython

Conversation

@andrewnester

@andrewnester andrewnester commented Feb 13, 2017

Copy link
Copy Markdown
Contributor

Fix for https://bugs.python.org/issue29534

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Feb 13, 2017
@the-knights-who-say-ni

the-knights-who-say-ni commented Feb 13, 2017

Copy link
Copy Markdown

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 is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

  1. If you don't have an account on b.p.o, please create one
  2. Make sure your GitHub username is listed in "Your Details" at b.p.o
  3. If you have not already done so, please sign the PSF contributor agreement
  4. If you just signed the CLA, please wait at least a day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
  5. Reply here saying you have completed the above steps

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag berkerpeksag changed the title Fixed #29534 - _decimal difference with _pydecimal bpo-29534: _decimal difference with _pydecimal Feb 13, 2017
@berkerpeksag
berkerpeksag requested a review from skrah February 13, 2017 10:27
@mdickinson

mdickinson commented Feb 13, 2017

Copy link
Copy Markdown
Member

The new behaviour seems like the right thing to me. Can you rework the code to avoid the repetition of the if cls is Decimal block?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andrewnester

andrewnester commented Feb 13, 2017

Copy link
Copy Markdown
Contributor Author

@mdickinson thanks for the comment! I've just reworked my PR to avoid code duplication.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mdickinson
mdickinson reviewed Feb 13, 2017
View reviewed changes

@mdickinson mdickinson left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Changes mostly LGTM, modulo a couple of nitpicks.

Still needs:

  • contributor agreement for Andrew
  • Misc/NEWS entry

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/_pydecimal.py Outdated

@mdickinson mdickinson Feb 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Style nit: please add a space after the >=.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_decimal.py Outdated

@mdickinson mdickinson Feb 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This change seems unrelated to the PR; please could you revert it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_decimal.py Outdated

@mdickinson mdickinson Feb 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is just nitpicking, but I think we're testing the wrong thing here: the change is in the type of the object that's passed to the Decimal subclass; the repr of the value was just a convenient way of demonstrating the difference in behaviours between the Python and C decimal implementations. How about recording and checking the type instead of the repr?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@andrewnester andrewnester Feb 13, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@mdickinson good idea, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@andrewnester

andrewnester commented Feb 13, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@mdickinson thanks! I've fixed all the nitpickings you mentioned. Also updated NEWS.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdickinson

mdickinson commented Feb 13, 2017

Copy link
Copy Markdown
Member

@andrewnester: Thank you! One last nitpick: please could you change the name of the test?

Apart from that, this looks good to go. Did you fill out the CLA?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andrewnester
Fixed #29534 - _decimal difference with _pydecimal
c030ca7
@andrewnester

andrewnester commented Feb 13, 2017

Copy link
Copy Markdown
Contributor Author

@mdickinson fixed. yes, I filled it couple of hours ago :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdickinson

mdickinson commented Feb 13, 2017

Copy link
Copy Markdown
Member

In, that case, LGTM unconditionally.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mdickinson
mdickinson approved these changes Feb 13, 2017
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@codecov

codecov Bot commented Feb 13, 2017

Copy link
Copy Markdown

Codecov Report

Merging #65 into master will decrease coverage by -0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
- Coverage   82.37%   82.37%   -0.01%     
==========================================
  Files        1427     1427              
  Lines      350948   350959      +11     
==========================================
+ Hits       289095   289100       +5     
- Misses      61853    61859       +6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cdbd68...c030ca7. Read the comment docs.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdickinson

mdickinson commented Feb 13, 2017

Copy link
Copy Markdown
Member

The coverage gods seem a bit peeved, but I can't make head or tail of the coverage report. Which 6 lines are new misses? (And which 5 lines are new hits?)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zware zware removed the CLA not signed label Feb 14, 2017
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 14, 2017
@mdickinson
mdickinson merged commit 6d1dece into python:master Feb 14, 2017
@mdickinson

mdickinson commented Feb 14, 2017

Copy link
Copy Markdown
Member

Merged. Thanks for the contribution!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdickinson mdickinson mentioned this pull request Feb 14, 2017
bpo-29534: move Misc/NEWS entry to correct section; add Misc/ACKS entry. #99
Merged
jaraco pushed a commit that referenced this pull request Dec 2, 2022
@pyup-bot @Mariatta
Update aiohttp from 3.0.1 to 3.0.2 (GH-65)
19b429b
sunmy2019 referenced this pull request in sunmy2019/cpython Apr 9, 2023
@pablogsal
Merge pull request #65 from lysnikolaou/more-fstring-errors
9a6e7e4
oraluben pushed a commit to oraluben/cpython that referenced this pull request Jul 14, 2023
@Fidget-Spinner @JuliaPoo
feat: execute jitted code in BB_BRANCH (python#65)
4768ece 
* refactor: return BB metadata from BB creation function

* feat: add execute jitted code in BB_BRANCH

* nit: disable JIT debug

* nit: Removed debugging comment in ceval_macros.h

* nit: Removed indirection warning in jit.py

---------

Co-authored-by: Jules <[email protected]>
@gvanrossum gvanrossum mentioned this pull request Aug 22, 2023
heap-use-after-free in _PyFunction_LookupByVersion #108253
Closed
@LinanV LinanV mentioned this pull request Oct 23, 2023
Objects/typeobject.c: No such file or directory. #111203
Closed
@crazymerlyn crazymerlyn mentioned this pull request Sep 22, 2024
Fix inconsistent behavior of Decimal.from_float RustPython/RustPython#5411
Merged
@ngoldbaum ngoldbaum mentioned this pull request Sep 23, 2024
Crash running PyO3 tests with --test-threads=1000 #124375
Closed
This was referenced Feb 11, 2025
segmentfault when pip installing setuptools #129992
Closed
segmentfault when pip installing setuptools #129993
Closed
AA-Turner pushed a commit to AA-Turner/cpython that referenced this pull request Apr 17, 2025
@davepeck
Remove interpolation_compare() and interpolation_hash() (python#65)
99c3bf4
@sergey-miryanov sergey-miryanov mentioned this pull request Dec 18, 2025
Observed memory leak in ssl library: Python 3.14 GC issue #142516
Closed
@ngoldbaum ngoldbaum mentioned this pull request Mar 6, 2026
Remove deprecated and undocumented function ctypes.SetPointerType #133866
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@andrewnester @the-knights-who-say-ni @mdickinson @zware
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.