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

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 }})

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:

Sign the PSF contributor agreement
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)
Reply here saying you have completed the above steps

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

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)

berkerpeksag changed the title~~bpo-29540 - Added json.COMPACT constant~~ bpo-29540: Add json.COMPACT constant

Feb 13, 2017

@@ -175,6 +175,8 @@ Basic Usage
.. versionchanged:: 3.4
Use ``(',', ': ')`` as default if indent is not ``None``.

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

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"


.. data:: compact

Constant that could be used as separators argument value

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.

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.

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.

@@ -175,6 +175,8 @@ Basic Usage
.. versionchanged:: 3.4
Use ``(',', ': ')`` as default if indent is not ``None``.

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

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.

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.

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`


.. data:: compact

Constant that could be used as separators argument value

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.

Choose a reason for hiding this comment

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

Constants
^^^^^^^^^

.. data:: compact

Choose a reason for hiding this comment

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

compact -> COMPACT


.. data:: compact

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

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.

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

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.

@@ -453,6 +467,9 @@ Encoders and Decoders
.. versionchanged:: 3.4
Use ``(',', ': ')`` as default if indent is not ``None``.

.. versionchanged:: 3.7
Instead of ``(',', ':')`` constant :data:`json.COMPACT` can be used.

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.

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!

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

brettcannon changed the title~~bpo-29540: Add json.COMPACT constant~~ bpo-29863: Add json.COMPACT constant

Mar 20, 2017

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().

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

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

Closing this since I just closed the issue.

jaraco pushed a commit that referenced this pull request

Dec 2, 2022