Skip to content

Conversation

@jasalt
Copy link

@jasalt jasalt commented Sep 22, 2025

Issue: #1275

Dismiss global PRINT_SEPARATOR (empty/whitespace by default?) as hash map is a special case and benefits generally of the separator between pairs for readability.

Demo:

make repl
basilisp.user=> {:1 :2 :3 :4 :5 :6 :7 :8}
{:5 :6, :7 :8, :3 :4, :1 :2}

I'm not familiar of philosophy behind PRINT_SEPARATOR so improvement ideas are welcome. Maybe this variable could be overridden only when it has the empty default value? That way hash map would use comma separator as exception by default and the separator could be changed if wanted (is that something people would need to do?).

Dismiss global PRINT_SEPARATOR (empty by default?) as hash map is
a special case and benefits generally of the separator between pairs
for readability.
@jasalt
Copy link
Author

jasalt commented Sep 22, 2025

Learning how these tests are set up, local make test output with Python 3.11.2 after poetry install:

======================================= short test summary info ========================================
FAILED tests/basilisp/cli_test.py::TestBootstrap::test_install_import - subprocess.CalledProcessError: Command '['/tmp/pytest-of-user/pytest-1/test_install_import0/venv/bi...
FAILED tests/basilisp/lrepr_test.py::test_print_length_maps - AssertionError: assert '{:b 2, ...}' in {'{:a 1 ...}', '{:b 2 ...}'}
FAILED tests/basilisp/map_test.py::test_map_repr - assert '{:key1 "val1", :key2 3}' in ['{:key2 3 :key1 "val1"}', '{:key1 "val1" :key2 3}']
FAILED tests/basilisp/test_core_fns.lpy::pr-test
FAILED tests/basilisp/test_defrecord.lpy::simple-defrecord-test
FAILED tests/basilisp/test_defrecord.lpy::defrecord-with-methods
================== 6 failed, 3771 passed, 1 skipped, 6 warnings in 939.74s (0:15:39) ===================

These seem relevant that would need edits:

  • tests/basilisp/lrepr_test.py::test_print_length_maps
  • tests/basilisp/map_test.py::test_map_repr
  • tests/basilisp/test_core_fns.lpy::pr-test

I'd be interested to hear feedback first about this in general.

@chrisrink10
Copy link
Member

Learning how these tests are set up, local make test output with Python 3.11.2 after poetry install:

======================================= short test summary info ========================================
FAILED tests/basilisp/cli_test.py::TestBootstrap::test_install_import - subprocess.CalledProcessError: Command '['/tmp/pytest-of-user/pytest-1/test_install_import0/venv/bi...
FAILED tests/basilisp/lrepr_test.py::test_print_length_maps - AssertionError: assert '{:b 2, ...}' in {'{:a 1 ...}', '{:b 2 ...}'}
FAILED tests/basilisp/map_test.py::test_map_repr - assert '{:key1 "val1", :key2 3}' in ['{:key2 3 :key1 "val1"}', '{:key1 "val1" :key2 3}']
FAILED tests/basilisp/test_core_fns.lpy::pr-test
FAILED tests/basilisp/test_defrecord.lpy::simple-defrecord-test
FAILED tests/basilisp/test_defrecord.lpy::defrecord-with-methods
================== 6 failed, 3771 passed, 1 skipped, 6 warnings in 939.74s (0:15:39) ===================

These seem relevant that would need edits:

* tests/basilisp/lrepr_test.py::test_print_length_maps

* tests/basilisp/map_test.py::test_map_repr

* tests/basilisp/test_core_fns.lpy::pr-test

I'd be interested to hear feedback first about this in general.

Sorry, I don't know how I missed notifications for this PR. Thanks for submitting it!

My guess is that the "unrelated" tests that are failing assert on some printed value that changed by adding the comma separator. Haven't had a chance to actually look though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants