Fixed: srepr not printing dict and set properly #19346

rational-kunal · 2020-05-17T12:23:33Z

References to other Issues or PRs

Fixes #19329

Brief description of what is fixed or changed

Added two methods _print_dict and _print_set to ReprPrinter.

Other comments

Release Notes

printing
- fixed a bug where srepr function would not print dictionary and set properly

sympy-bot · 2020-05-17T12:23:35Z

✅

Hi, I am the SymPy bot (v158). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

printing
- fixed a bug where srepr function would not print dictionary and set properly (#19346 by @rational-kunal)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #19329


#### Brief description of what is fixed or changed
Added two methods `_print_dict` and `_print_set` to `ReprPrinter`.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->
<!-- BEGIN RELEASE NOTES -->
* printing
  * fixed a bug where `srepr` function would not print dictionary and set properly
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

eric-wieser · 2020-05-17T13:20:52Z

sympy/printing/repr.py

@@ -144,6 +144,14 @@ def _print_EmptySequence(self, expr):
    def _print_list(self, expr):
        return "[%s]" % self.reprify(expr, ", ")

+    def _print_dict(self, expr):
+        sep = ", "
+        dict_kvs = ["%s: %s" % (self.doprint(key), self.doprint(value)) for key, value in expr.items()]


Most of the other implementations use _print I think

Suggested change

dict_kvs = ["%s: %s" % (self.doprint(key), self.doprint(value)) for key, value in expr.items()]

dict_kvs = ["%s: %s" % (self._print(key), self._print(value)) for key, value in expr.items()]

Although I suppose reprify does not, so this doesn't really matter.

eric-wieser · 2020-05-17T13:22:15Z

sympy/printing/repr.py

+        return "{%s}" % sep.join(dict_kvs)
+
+    def _print_set(self, expr):
+        return "{%s}" % self.reprify(expr, ", ")


This is wrong, and prints empty sets as empty dictionaries

Suggested change

return "{%s}" % self.reprify(expr, ", ")

if not expr:

return "set()"

else:

return "{%s}" % self.reprify(expr, ", ")

Edit: Done, thanks!

eric-wieser · 2020-05-17T13:22:40Z

sympy/printing/tests/test_repr.py

+def test_set():
+    from sympy import srepr
+    from sympy.abc import x, y
+    s = {x}


Suggested change

s = {x}

s = {}

assert srepr(s) == "set()"

s = {x}

eric-wieser · 2020-05-17T13:23:09Z

Generally looks good, just a mistake on empty sets

rational-kunal · 2020-05-17T13:29:32Z

@eric-wieser, should empty dict be printed differently

eric-wieser · 2020-05-17T13:34:37Z

No, empty dict is correct. The concern is that {} in python is an empty dict, not an empty set.

eric-wieser · 2020-05-17T13:45:47Z

sympy/printing/tests/test_repr.py

+    d = {x: y}
+    assert srepr(d) == "{Symbol('x'): Symbol('y')}"
+    d = {x: y, y: z}
+    assert srepr(d) == "{Symbol('x'): Symbol('y'), Symbol('y'): Symbol('z')}"


This test won't work in Python 3.5, because dicts are not ordered. You need:

Suggested change

assert srepr(d) == "{Symbol('x'): Symbol('y'), Symbol('y'): Symbol('z')}"

# dict order is arbitrary until CPython 3.6 or Python 3.7

assert srepr(d) in (

"{Symbol('x'): Symbol('y'), Symbol('y'): Symbol('z')}",

"{Symbol('y'): Symbol('z'), Symbol('x'): Symbol('y')}",

)

@rational-kunal, CI will randomly fail unless you also make this change.

eric-wieser · 2020-05-17T13:46:30Z

sympy/printing/tests/test_repr.py

+    s = {x}
+    assert srepr(s) == "{Symbol('x')}"
+    s = {x, y}
+    assert srepr(s) == "{Symbol('x'), Symbol('y')}"


CI is failing from this test, because sets are not ordered.

Suggested change

assert srepr(s) == "{Symbol('x'), Symbol('y')}"

# set order is arbitrary

assert srepr(s) in (

"{Symbol('x'), Symbol('y')}",

"{Symbol('y'), Symbol('x')}",

)

I didn't account for that, thanks!

codecov · 2020-05-17T16:52:26Z

Codecov Report

Merging #19346 into master will increase coverage by 0.002%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #19346       +/-   ##
=============================================
+ Coverage   75.598%   75.601%   +0.002%     
=============================================
  Files          651       651               
  Lines       169483    169491        +8     
  Branches     39988     39990        +2     
=============================================
+ Hits        128127    128137       +10     
+ Misses       35744     35742        -2     
  Partials      5612      5612

eric-wieser · 2020-05-18T10:29:11Z

CI failure is an unrelated network issue

oscarbenjamin · 2020-05-19T22:45:27Z

Looks good. Thanks

fixed: srepr not printing dict and set properly

b8b80cf

eric-wieser reviewed May 17, 2020

View reviewed changes

handled printing empty set with srepr

398751d

eric-wieser reviewed May 17, 2020

View reviewed changes

rational-kunal added 2 commits May 17, 2020 19:20

fixed issue with printing sets

90e549c

fixed arbitary test fail due to dictionary

2a7b8a4

eric-wieser approved these changes May 17, 2020

View reviewed changes

czgdp1807 added the printing label May 17, 2020

oscarbenjamin merged commit 3e33c30 into sympy:master May 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed: srepr not printing dict and set properly #19346

Fixed: srepr not printing dict and set properly #19346

rational-kunal commented May 17, 2020 •

edited

sympy-bot commented May 17, 2020 •

edited

eric-wieser May 17, 2020

eric-wieser May 17, 2020

eric-wieser May 17, 2020 •

edited

eric-wieser May 17, 2020

eric-wieser commented May 17, 2020

rational-kunal commented May 17, 2020 •

edited

eric-wieser commented May 17, 2020 •

edited

eric-wieser May 17, 2020 •

edited

eric-wieser May 17, 2020

eric-wieser May 17, 2020 •

edited

rational-kunal May 17, 2020

codecov bot commented May 17, 2020 •

edited

eric-wieser commented May 18, 2020

oscarbenjamin commented May 19, 2020

	dict_kvs = ["%s: %s" % (self.doprint(key), self.doprint(value)) for key, value in expr.items()]
	dict_kvs = ["%s: %s" % (self._print(key), self._print(value)) for key, value in expr.items()]

-    s = {x}
+    s = {}
+    assert srepr(s) == "set()"
+    s = {x}

-    assert srepr(d) == "{Symbol('x'): Symbol('y'), Symbol('y'): Symbol('z')}"
+    # dict order is arbitrary until CPython 3.6 or Python 3.7
+    assert srepr(d) in (
+        "{Symbol('x'): Symbol('y'), Symbol('y'): Symbol('z')}",
+        "{Symbol('y'): Symbol('z'), Symbol('x'): Symbol('y')}",
+    )

-    assert srepr(s) == "{Symbol('x'), Symbol('y')}"
+    # set order is arbitrary
+    assert srepr(s) in (
+        "{Symbol('x'), Symbol('y')}",
+        "{Symbol('y'), Symbol('x')}",
+    )

Fixed: srepr not printing dict and set properly #19346

Fixed: srepr not printing dict and set properly #19346

Conversation

rational-kunal commented May 17, 2020 • edited

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented May 17, 2020 • edited

eric-wieser May 17, 2020

Choose a reason for hiding this comment

eric-wieser May 17, 2020

Choose a reason for hiding this comment

eric-wieser May 17, 2020 • edited

Choose a reason for hiding this comment

eric-wieser May 17, 2020

Choose a reason for hiding this comment

eric-wieser commented May 17, 2020

rational-kunal commented May 17, 2020 • edited

eric-wieser commented May 17, 2020 • edited

eric-wieser May 17, 2020 • edited

Choose a reason for hiding this comment

eric-wieser May 17, 2020

Choose a reason for hiding this comment

eric-wieser May 17, 2020 • edited

Choose a reason for hiding this comment

rational-kunal May 17, 2020

Choose a reason for hiding this comment

codecov bot commented May 17, 2020 • edited

Codecov Report

eric-wieser commented May 18, 2020

oscarbenjamin commented May 19, 2020

rational-kunal commented May 17, 2020 •

edited

sympy-bot commented May 17, 2020 •

edited

eric-wieser May 17, 2020 •

edited

rational-kunal commented May 17, 2020 •

edited

eric-wieser commented May 17, 2020 •

edited

eric-wieser May 17, 2020 •

edited

eric-wieser May 17, 2020 •

edited

codecov bot commented May 17, 2020 •

edited