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

Separate out the data from the table, allow inline tables and row seps. #72

Merged
merged 4 commits into from
Mar 6, 2018

Conversation

tpapp
Copy link
Collaborator

@tpapp tpapp commented Feb 24, 2018

Now TableData can be used as just the tabular data without options etc, allowing inline tables in [] options. Using \\ for row separators is possible.

This also makes the code cleaner, as TableData takes the place of table_fields; TableFile is also removed as now Table can contain a TableData or a string (filename).

Closes #65, added example for patch plots.

Printing inline tables in options required some printing code changes, which resulted in some incidental changes:

  • Printing 0:1 as "0:1" was a fluke (cf 1.2:3.5). Fixed this in the examples.

  • Added space before [] (options), this is the dominant usage in the manual. Misc other whitespace changes.

  • Removed indentation from some print_tex methods, as it was unnecessary (but innocuous).

  • print_tex vectors as { ... } lists (was not used for anything anyway).

  • print_indent was adding an extra newline at the end, fixed.

Now TableData can be used as just the tabular data without options
etc, allowing inline tables in `[]` options. Using `\\` for row
separators is possible.

This also makes the code cleaner, as `TableData` takes the place of
`table_fields`; `TableFile` is also removed as now `Table` can contain
a `TableData` or a string (filename).

Closes #65, added example for patch plots.

Printing inline tables in options required some printing code changes,
which resulted in some incidental changes:

- Printing `0:1` as "0:1" was a fluke (cf `1.2:3.5`). Fixed this in
the examples.

- Added space before `[]` (options), this is the dominant usage
in the manual. Misc other whitespace changes.

- Removed indentation from some `print_tex` methods, as it was
unnecessary (but innocuous).

- `print_tex` vectors as `{ ... }` lists (was not used for anything
  anyway).

- `print_indent` was adding an extra newline at the end, fixed.
@tpapp
Copy link
Collaborator Author

tpapp commented Feb 24, 2018

Note that doctests are broken (again), but this does not break tests. I plan to handle them in #73.

@codecov-io
Copy link

codecov-io commented Feb 24, 2018

Codecov Report

Merging #72 into master will increase coverage by 0.37%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   81.49%   81.87%   +0.37%     
==========================================
  Files           9        9              
  Lines         481      480       -1     
==========================================
+ Hits          392      393       +1     
+ Misses         89       87       -2
Impacted Files Coverage Δ
src/tikzdocument.jl 54.67% <ø> (-0.33%) ⬇️
src/requires.jl 84.84% <100%> (ø) ⬆️
src/utilities.jl 95.65% <100%> (+2.79%) ⬆️
src/PGFPlotsX.jl 84.61% <100%> (-3.62%) ⬇️
src/options.jl 93.75% <100%> (+2.44%) ⬆️
src/axiselements.jl 94.26% <100%> (+0.36%) ⬆️
src/axislike.jl 92.85% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4c7145...f3f32f5. Read the comment docs.

@tpapp
Copy link
Collaborator Author

tpapp commented Mar 5, 2018

@KristofferC : friendly ping. I realize you may be busy on other fronts and not have time for a review, should I wait or OK if I merge? I would get started on #73 and #59.

Copy link
Owner

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

Sorry for slow review, some comments.

@@ -138,7 +138,7 @@ Example:
julia> p = @pgf Plot(Table("plotdata/invcum.dat"), { blue }; incremental = false);

julia> print_tex(p)
\addplot[blue]
\addplot [blue]
Copy link
Owner

Choose a reason for hiding this comment

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

Why add this space? That doesn't seem like the convention used in e.g. http://pgfplots.sourceforge.net/gallery.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, will remove.

src/PGFPlotsX.jl Outdated
print(io, str)
end
end
print_tex(io::IO, str::AbstractString) = print(io, str)
Copy link
Owner

Choose a reason for hiding this comment

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

These will now not respect the indentation at all e.g printed completely to left aligned? What's the gain of changing this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this used anywhere? It is my impression that it was used to print "inline" representations, not preceded by newlines.

Copy link
Owner

Choose a reason for hiding this comment

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

I would assume that this is used when there is a string in an Axis for example. And then that string would have the correct indentation.

src/PGFPlotsX.jl Outdated
$SIGNATURES

Print vectors as comma-delimited lists between `{}`s. Useful for eg
`/pgfplots/xtick`.
Copy link
Owner

Choose a reason for hiding this comment

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

If you want to use this in the options you should overload print_opt? And for vector it is defined to pint a comma-delimited list:

print_opt(io::IO, v::Vector) = print(io, join(v, ","))

@@ -296,7 +296,7 @@ savefigs("spline-quadratic", ans) # hide
mesh,
scatter,
samples = 10,
domain = 0:1
domain = "0:1"
Copy link
Owner

Choose a reason for hiding this comment

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

I wish we could fix this to work in general, could be done later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made a reminder in #74.

It is not used outside options.

Fix examples which call `Axis` with vectors.

Document `print_options`.

Add tests for AbstractVector in options.
@tpapp
Copy link
Collaborator Author

tpapp commented Mar 5, 2018

Working on fixing printing, let's not merge yet (even if tests pass).

Partial fix for #73.

Make containers indent their contents, all print as if at top level.
This simplifies `print_tex` methods considerably, and leads to nicer
output.

Factor out `add_indent`, making it testable.

Coordinates and table lines are indented.

Allow space instead of newlines for options, use this for single-line
expressions (Legend, Graphics, etc).

Minor fix: order of arguments of `Graphics` was inconsistent with
other `OptionType`s.

Unit test a bare minimum of emitted strings; this verges on
overtesting but is much nicer to debug than failures in doctests.
@tpapp
Copy link
Collaborator Author

tpapp commented Mar 5, 2018

Sorry for the mess, with f3f32f5 this is now a very large PR which should have been 2-3 PRs, but was easier to go forward than backward. Now everything (incl printing) is fixed and doctests run without any problems.

@KristofferC
Copy link
Owner

Looks good, nice job :)

@tpapp tpapp merged commit a7af467 into master Mar 6, 2018
@tpapp tpapp deleted the tp/tabledata branch March 6, 2018 11:41
@tpapp tpapp mentioned this pull request Mar 12, 2018
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.

row sep in tables.
3 participants