Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #2894 via ToStringIndent() #2909

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Dunbaratu
Copy link
Member

@Dunbaratu Dunbaratu commented Mar 27, 2021

Fixes #2894

There are going to be two proposals for how to fix #2894. (Read the comments for the issue).

This is one of them.

In this proposal, We stop borrowing the Dump infrastructure for
creating ToString()s. The Dump infrastructure was designed for
when you want to serialize, meaing every single field must be
visited and written with nothing 'missing', so values can be
saved and restored. I found that layering on top of that
system while still preserving the notion of ToString() being meant
for human consumption was kind of hard to wrap my head around
so I went with this idea.

This PR is being made before this is fully tested, so it can be
shown to other people who are proposing alternate solutions. I
also want to show what this output ends up looking like when you
try it. Note the code still has some "eraseme" things in it
at this point, which is how I remind myself that an idea wasn't
done yet. (I have a check I do before a final PR, usually, where
I 'grep' all my code for the string "eraseme" to highlight places
I stuck something in temporarily during testing. Here I'm using
that to remind myself of stuff that isn't done yet in this PR.)

While I think this produces good output, I am willing to be convinced
other approaches might work too, so I'm open to trying to preserve
using Dumper for indented ToString()s if it can be done.

Fixes KSP-KOS#2894

There are going to be two proposals for how to fix KSP-KOS#2894. (Read the
comments for the issue).

This is one of them.

In this proposal, We stop borrowing the Dump infrastructure for
creating ToString()s.  The Dump infrastructure was designed for
when you want to serialize, meaing every single field must be
visited and written with nothing 'missing', so values can be
saved and restored.  I found that layering on top of that
system while still preserving the notion of ToString() being meant
for human consumption was kind of hard to wrap my head around
so I went with this idea.

This PR is being made before this is fully tested, so it can be
shown to other people who are proposing alternate solutions.  I
also want to show what this output ends up looking like when you
try it.  Note the code still has some "eraseme" things in it
at this point, which is how I remind myself that an idea wasn't
done yet.  (I have a check I do before a final PR, usually, where
I 'grep' all my code for the string "eraseme" to highlight places
I stuck something in temporarily during testing.  Here I'm using
that to remind myself of stuff that isn't done yet in this PR.

While I think this produces good output, I am willing to be convinced
other approaches might work too, so I'm open to trying to preserve
using Dumper for indented ToString()s if it can be done.
@Dunbaratu Dunbaratu added the bug Weird outcome is probably not what the mod programmer expected. label Mar 27, 2021
@Dunbaratu
Copy link
Member Author

Here is an example program and the output it makes with this PR.

Example program:

set L1 to LIST(100,200,300,400,500,600,LIST("a","b","c"),"Hello").
set L2 to LIST(10, 20, 30, 40, 50, 60, "inner 1", "inner 2", red, green, blue, range(0,50,2)).
// Let's nest L2 inside L3:
set L3 to LEX("key 1", "value 1", "key 2", "value 2", "key 3", L2).
set L4 to QUEUE("A", "B", "C").
set L5 to STACK("A", "B", "C").
L1:add(L3).
L1:add(L4).
L1:add(L5).
L1:add(LIST()).
L1:add(LIST("one thing")).
L1:add("just one last thing").
print L1.

With output like this:
screenshot27

@Dunbaratu
Copy link
Member Author

Dunbaratu commented Mar 27, 2021

Hah I just realized I used the terminology "top ->" when showing a Queue and "front ->" when showing a Stack. Those terms should be swapped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Weird outcome is probably not what the mod programmer expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Printing containers (lex, list) is now overly verbose
1 participant