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-29863: Add json.COMPACT constant by andrewnester · Pull Request #72 · python/cpython

Conversation

@andrewnester

@andrewnester andrewnester commented Feb 13, 2017

Copy link
Copy Markdown
Contributor

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

I decided to go with Alternative version provided R.David Murray just to keep things simple

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 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 these steps to rectify the issue:

  1. Sign the PSF contributor agreement
  2. 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)
  3. 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 requested changes Feb 13, 2017
View reviewed changes

@berkerpeksag berkerpeksag 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.

Thanks for the PR. The new json.COMPACT constant needs to be documented in Doc/library/json.rst (and please add a .. versionadded:: 3.7 marker)

Sorry, something went wrong.

Uh oh!

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

All reactions
@berkerpeksag berkerpeksag changed the title bpo-29540 - Added json.COMPACT constant bpo-29540: Add json.COMPACT constant Feb 13, 2017
@andrewnester

andrewnester commented Feb 13, 2017

Copy link
Copy Markdown
Contributor Author

@berkerpeksag thanks! just added documentation for json.COMPACT

All reactions

Sorry, something went wrong.

Uh oh!

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

brettcannon
brettcannon requested changes Feb 13, 2017
View reviewed changes
Comment thread Doc/library/json.rst Outdated

@brettcannon brettcannon 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.

"could be used" -> "can be used"

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/library/json.rst Outdated

@brettcannon brettcannon 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.

A constant that can be used as the *separators* argument to emit a compact serialization.

Sorry, something went wrong.

Uh oh!

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

All reactions

@berkerpeksag berkerpeksag 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.

+1 to Brett's suggestion.

Sorry, something went wrong.

Uh oh!

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

All reactions
berkerpeksag
berkerpeksag reviewed Feb 13, 2017
View reviewed changes

@berkerpeksag berkerpeksag 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.

Please update the "Compact encoding" example at the top of the page.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/library/json.rst Outdated

@berkerpeksag berkerpeksag 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.

Also, following the style used in Doc/library/json.rst would be better:

.. versionadded:: 3.7
   Instead of ``(',', ':')`` constant ``json.COMPACT`` could be used.

Sorry, something went wrong.

Uh oh!

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

All reactions

@berkerpeksag berkerpeksag 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.

versionchanged would be more appropriate here.

Sorry, something went wrong.

Uh oh!

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

All reactions

@berkerpeksag berkerpeksag 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.

``json.COMPACT`` -> :data:`json.COMPACT`

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/library/json.rst Outdated

@berkerpeksag berkerpeksag 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.

+1 to Brett's suggestion.

Sorry, something went wrong.

Uh oh!

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

All reactions
@andrewnester

andrewnester commented Feb 14, 2017

Copy link
Copy Markdown
Contributor Author

@berkerpeksag @brettcannon thanks! I've just updated PR according to changes you requested.

All reactions

Sorry, something went wrong.

Uh oh!

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

berkerpeksag
berkerpeksag reviewed Feb 14, 2017
View reviewed changes

@berkerpeksag berkerpeksag 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.

Please also update JSONEncoder documentation at https://docs.python.org/3/library/json.html#json.JSONEncoder and add a test that passes separators=json.COMPACT to JSONEncoder. Sorry, I missed this in my earlier review.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/library/json.rst Outdated

@berkerpeksag berkerpeksag Feb 14, 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.

compact -> COMPACT

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/library/json.rst Outdated

@berkerpeksag berkerpeksag Feb 14, 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.

Please wrap lines at 80 chars.

Sorry, something went wrong.

Uh oh!

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

All reactions
@andrewnester

andrewnester commented Feb 14, 2017

Copy link
Copy Markdown
Contributor Author

thanks @berkerpeksag ! I've just added changes corresponding to JSONEncoder

All reactions

Sorry, something went wrong.

Uh oh!

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

@codecov

codecov Bot commented Feb 14, 2017

Copy link
Copy Markdown

Codecov Report

Merging #72 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   82.38%   82.38%   +<.01%     
==========================================
  Files        1428     1428              
  Lines      351138   351147       +9     
==========================================
+ Hits       289282   289294      +12     
+ Misses      61856    61853       -3

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 d50f188...71c48e2. Read the comment docs.

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
brettcannon
brettcannon requested changes Feb 15, 2017
View reviewed changes
Comment thread Doc/library/json.rst Outdated

@brettcannon brettcannon Feb 15, 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.

I don't think a versionchanged field is necessary as nothing about the function actually changed. I think it would be better to update the paragraph above from "To get the most compact JSON representation,
you should specify (',', ':') to eliminate whitespace." to "To get the most compact JSON representation,
you should specify :attr:json.COMPACT to eliminate whitespace.

Sorry, something went wrong.

Uh oh!

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

All reactions

@andrewnester andrewnester Feb 16, 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.

@brettcannon sounds reasonable! I've just updated it, thanks!

Sorry, something went wrong.

Uh oh!

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

All reactions
andrewnester added 3 commits February 16, 2017 16:20
@andrewnester
29540 - Added json.COMPACT constant
cc0a1f1
@andrewnester
Updated documentation for json.COMPACT
32c4b13
@andrewnester
Added test to ensure JSONEncoder works with json.COMPACT.
4315176
@andrewnester

andrewnester commented Feb 22, 2017

Copy link
Copy Markdown
Contributor Author

@berkerpeksag @brettcannon any conclusion on this? :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@brettcannon

brettcannon commented Feb 24, 2017

Copy link
Copy Markdown
Member

@andrewnester I've been waiting for the discussion of https://bugs.python.org/issue29540 to settle down and be resolved before commenting further.

All reactions

Sorry, something went wrong.

Uh oh!

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

@andrewnester

andrewnester commented Mar 12, 2017

Copy link
Copy Markdown
Contributor Author

@brettcannon looks like discussion stopped here https://bugs.python.org/issue29540 ..
any conclusion on this PR?

All reactions

Sorry, something went wrong.

Uh oh!

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

@brettcannon

brettcannon commented Mar 13, 2017

Copy link
Copy Markdown
Member

@andrewnester I just pinged the issue to explicitly see what people think about your attribute proposal.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brettcannon brettcannon changed the title bpo-29540: Add json.COMPACT constant bpo-29863: Add json.COMPACT constant Mar 20, 2017
@brettcannon

brettcannon commented Mar 20, 2017

Copy link
Copy Markdown
Member

I opened bpo-29863 to finalize the discussion of the constant since I think it got lost on the older issue that proposed a new compact argument to json.dump().

All reactions

Sorry, something went wrong.

Uh oh!

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

@andrewnester

andrewnester commented Apr 2, 2017

Copy link
Copy Markdown
Contributor Author

@brettcannon thanks! is there any link to discussion or some mailing list?

All reactions

Sorry, something went wrong.

Uh oh!

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

@orsenthil

orsenthil commented Apr 2, 2017

Copy link
Copy Markdown
Member

@andrewnester : http://bugs.python.org/issue29863

andrewnester reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@brettcannon

brettcannon commented Apr 3, 2017

Copy link
Copy Markdown
Member

@andrewnester I just wanted to say sorry for the delay on making a decision for this. As I'm sure you have noticed on the issue it's basically split down the middle as to whether this PR should be accepted or not and so no one has made the final call.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brettcannon

brettcannon commented May 13, 2017

Copy link
Copy Markdown
Member

Closing this since I just closed the issue.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brettcannon brettcannon closed this May 13, 2017
@Askaholic Askaholic mentioned this pull request Jul 22, 2019
Server bothers with whitespace in json messages FAForever/server#166
Closed
jaraco pushed a commit that referenced this pull request Dec 2, 2022
@pyup-bot @Mariatta
Update aiohttp from 3.0.6 to 3.0.7 (#72)
61b0861
jaraco added a commit to jaraco/cpython that referenced this pull request Feb 17, 2023
@jaraco
Ignore the type. Fixes emergent qa failure and closes python#72.
be21e5a
oraluben pushed a commit to oraluben/cpython that referenced this pull request Jul 14, 2023
@Fidget-Spinner
feat: executed jitted code for all BB_BRANCH and BB_JUMP (python#72)
81e84a4
@gvanrossum gvanrossum mentioned this pull request Aug 22, 2023
heap-use-after-free in _PyFunction_LookupByVersion #108253
Closed
lysnikolaou referenced this pull request in lysnikolaou/cpython Apr 30, 2024
@pablogsal
Merge pull request #72 from lysnikolaou/pyrepl-refactor-term-unix-con…
aa9eaf3
…sole

Refactor termios stuff in unix console
@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 added a commit to AA-Turner/cpython that referenced this pull request Apr 22, 2025
@AA-Turner @picnixz
t-strings: Reduce undefined behaviour and general improvements (pytho…
5782897
…n#72)

Co-authored-by: Bénédikt Tran <[email protected]>
@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

@berkerpeksag berkerpeksag berkerpeksag requested changes
@brettcannon brettcannon brettcannon requested changes

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.

6 participants

@andrewnester @the-knights-who-say-ni @brettcannon @orsenthil @berkerpeksag @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.