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.

Sign up for GitHub

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

Jump to bottom

Bind *print-length* to false in EDN serde #22

Merged
merged 1 commit into from
Jan 4, 2019
Merged

Bind *print-length* to false in EDN serde #22

merged 1 commit into from
Jan 4, 2019

Conversation

r0man
Copy link
Contributor

@r0man r0man commented Dec 27, 2018

Collections with more than 100 elements currently get truncated, due
to print-length being bound to 100 by default.

This patch binds the var to false when printing in the EDN serde,
which makes round-tripping of larger collections possible.

@r0man r0man requested a review from a team as a code owner December 27, 2018 12:44
arrdem
arrdem reviewed Dec 27, 2018
View reviewed changes
src/jackdaw/serdes/edn.clj Outdated
@@ -23,7 +23,9 @@
[]
(jsfn/new-serializer {:serialize (fn [_ _ data]
(when data
(to-bytes (prn-str data))))}))
(to-bytes
(binding [*print-length* false]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also bind *print-level*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arrdem done

arrdem
arrdem approved these changes Dec 27, 2018
View reviewed changes
Copy link
Contributor

@arrdem arrdem left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again!

@r0man
Copy link
Contributor Author

r0man commented Dec 28, 2018

Thanks, as well for jackdaw!

@AndreaCrotti
Copy link
Contributor

Hi @r0man and thanks for your PR, we just fixed the build now for external PRs, could you please rebase your branch on master to allow CircleCI to do its job?
Thanks

@r0man
Copy link
Contributor Author

r0man commented Jan 2, 2019

@AndreaCrotti done

@r0man r0man force-pushed the print-length branch 3 times, most recently from b33f489 to f8cd937 Compare January 2, 2019 11:56
@r0man
Copy link
Contributor Author

r0man commented Jan 2, 2019

Looks like *print-length* is bound to a number in the cider, but to nil when running the tests from the command line. I fixed this by binding it in the test.

@99-not-out 99-not-out mentioned this pull request Jan 4, 2019
Merged
@99-not-out
Copy link
Contributor

@r0man ... and another rebase, now I merged #21 ...

@r0man
Bind *print-length* and *print-level* to false in EDN serde
456f086 
Collections with more than 100 elements currently get truncated, due
to *print-length* being bound to 100 by default.

This patch binds the var to false when printing in the EDN serde,
which makes round-tripping of larger collections possible.
@r0man
Copy link
Contributor Author

r0man commented Jan 4, 2019

@99-not-out done

@r0man
Copy link
Contributor Author

r0man commented Jan 4, 2019

@99-not-out Hmm, now the build failed, because of the gen/any-printable producing #NaN sometimes. This comes from the double generator used by any-printable. It could also produce Infinity which I think is not compareable with =.

@r0man
Copy link
Contributor Author

r0man commented Jan 4, 2019

This was the failing build: https://circleci.com/gh/FundingCircle/jackdaw/65?utm_campaign=workflow-failed&utm_medium=email&utm_source=notification

@99-not-out
Copy link
Contributor

Ah - I will raise an issue and merge this PR based on the issue being a pre-existing one.
Thanks @r0man!

@99-not-out 99-not-out merged commit a0987cb into FundingCircle:master Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arrdem arrdem arrdem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@r0man @AndreaCrotti @99-not-out @arrdem