basilisp-lang / basilisp

A Clojure-compatible(-ish) Lisp dialect targeting Python 3.8+
https://basilisp.readthedocs.io
Eclipse Public License 1.0
256 stars 6 forks source link

*print-namespace-maps* support #883

Closed ikappaki closed 6 months ago

ikappaki commented 7 months ago

Hi,

could you please review draft support for the print-namespace-maps dynamic variable. It resolves #882.

I implemented the support at the lowest obj.py level, though this required to bring in the keyword.py and symbol.py modules resulting to cyclic dependencies. Not sure how else to implement it, so any other suggestions are most welcome.

I've also blindly followed the example of print-dup on how I ended setup the variable in the cli/runtime/object .py files.

In addition, I've used a set to compare the printed output of the map result with multiple k/v, exhaustively covering the possible orderings, is there perhaps a better simpler way to do this? This currently is prune to user errors.

Thanks

ikappaki commented 7 months ago

I appreciate your attempting to resolve the circular dependency issue, but my concern is that we now have duplicated logic for displaying map Lisp representations in two places and my feeling is that is far more likely to cause issues than the circular import.

Reverted back to the earlier commit, and addressed all of the points made so far. I have to introduce a py_map option to map_lrepr fn to prevent python dictionary being printed with namespace when that option is on.

chrisrink10 commented 6 months ago

I appreciate your attempting to resolve the circular dependency issue, but my concern is that we now have duplicated logic for displaying map Lisp representations in two places and my feeling is that is far more likely to cause issues than the circular import.

Reverted back to the earlier commit, and addressed all of the points made so far. I have to introduce a py_map option to map_lrepr fn to prevent python dictionary being printed with namespace when that option is on.

I initially had this reaction, but I'm not sure I think there's any reason to prevent python dicts from being printed using the same logic? They can be read with a namespace prefix:

basilisp.user=> #py #:abc{:def 1 "c" 3}
#py {"c" 3 :abc/def 1}
ikappaki commented 6 months ago

I appreciate your attempting to resolve the circular dependency issue, but my concern is that we now have duplicated logic for displaying map Lisp representations in two places and my feeling is that is far more likely to cause issues than the circular import.

Reverted back to the earlier commit, and addressed all of the points made so far. I have to introduce a py_map option to map_lrepr fn to prevent python dictionary being printed with namespace when that option is on.

I initially had this reaction, but I'm not sure I think there's any reason to prevent python dicts from being printed using the same logic? They can be read with a namespace prefix:

basilisp.user=> #py #:abc{:def 1 "c" 3}
#py {"c" 3 :abc/def 1}

Right, I've reverted that particular change and python dicts now print out the same as Clojure maps.

chrisrink10 commented 6 months ago

Can you rebase against the other PR? I will approve afterwards.

ikappaki commented 6 months ago

Can you rebase against the other PR? I will approve afterwards.

Done