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

[print] Fix map entries being spliced in non-map collections #292

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

alexander-yakushev
Copy link
Member

@alexander-yakushev alexander-yakushev commented Aug 28, 2024

Fixes #291.


@alexander-yakushev alexander-yakushev force-pushed the print-map-entry branch 3 times, most recently from 2577b38 to 4dd15c3 Compare August 28, 2024 08:17
(defn- print-coll [^TruncatingStringWriter w, ^Iterable x, ^String sep
^String prefix, ^String suffix]
^String prefix, ^String suffix, map?]
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of this boolean param, but I can live with it. Perhaps we can at least make it somewhat optional, so we don't have to pass false around.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated it to be optional. Does it look that much better for you? It's a private function anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Not much better, but a bit better - in general I'm not a big fan of boolean params, as I always have to consult the method signature to remember/understand what some boolean flag is supposed to do. But I agree it's not a big deal in our case, given it's a private API.

@alexander-yakushev alexander-yakushev merged commit d8b31d4 into master Aug 28, 2024
24 checks passed
@alexander-yakushev alexander-yakushev deleted the print-map-entry branch August 28, 2024 09:04
@yuhan0
Copy link
Contributor

yuhan0 commented Aug 28, 2024

Thanks for the quick fix! Note there's a typo in the changelog, 290 -> 292

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.

orchard.print renders map entries in a misleading way
3 participants