[BREAKING] Circular ref bug show #2200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nalimilan
reviewed
Apr 21, 2020
I would leave it for a separate PR (as it probably will lead to a cascade of other things to check - I am just not sure that I can get it 100% right then - these |
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
…ular_ref_bug_show
|
All suggestions are applied. I think the major decision is what to do with |
nalimilan
approved these changes
Apr 24, 2020
|
Thank you. I will merge it after #2911 is done. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2135
This is not a full fix, as Base is much better at detecting circular references, but doing it right would require a much bigger rewrite of the code. Now we correctly handle 1-level of indirection (but do not correctly handle a case when e.g. a data frame would contain a vector containing a dict containing the original data frame - but I think this case is so obscure that it can be skipped).
Additionally now we:
missing,nothingand#undefconsistently everywhere (text, LaTeX, HTML, CSV, TSV) - the only restriction I left is CSV and TSV throwing an error when trying to write #undefmissing,nothingand#undefeverywhere (text, LaTeX, HTML) - so they are never mixed up with stringsAbstractDataFrame,DataFrameRow,DataFrameRows,DataFrameColumnsorGroupedDataFrameis stored in a data frame then only its summary is printed (this makes printing much nicer (otherwise the output were not readable anyway) - and as a side effect - avoids the original circular reference problem)Finally this PR fixes a problem that old printing mechanism in DataFrames.jl was not thread safe. After fixing it is thread safe (we do not reuse one global
bufferbut allocate it anew for each top-level call ofshow).This PR is largely orthogonal to #2199 and can be merged post 0.21 release, but before 1.0 release as it is mildly breaking.