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

PGFPlotsX: reworks #4262

Merged
merged 4 commits into from
Jul 24, 2022
Merged

PGFPlotsX: reworks #4262

merged 4 commits into from
Jul 24, 2022

Conversation

t-bltg
Copy link
Member

@t-bltg t-bltg commented Jul 7, 2022

Needs korsbo/Latexify.jl#223.

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #4262 (6bd4db4) into master (1cafdb0) will increase coverage by 0.24%.
The diff coverage is 71.23%.

@@            Coverage Diff             @@
##           master    #4262      +/-   ##
==========================================
+ Coverage   79.67%   79.91%   +0.24%     
==========================================
  Files          29       29              
  Lines        7052     6962      -90     
==========================================
- Hits         5619     5564      -55     
+ Misses       1433     1398      -35     
Impacted Files Coverage Δ
src/backends/pgfplotsx.jl 71.84% <70.96%> (+1.24%) ⬆️
src/axes.jl 86.43% <100.00%> (+0.43%) ⬆️
src/colorbars.jl 87.50% <100.00%> (-0.20%) ⬇️

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 1cafdb0...6bd4db4. Read the comment docs.

@BeastyBlacksmith
Copy link
Member

can you split the functional and the aestetics part of this PR into two PRs?

@t-bltg
Copy link
Member Author

t-bltg commented Jul 11, 2022

can you split the functional and the aestetics part of this PR into two PRs?

I've rebased the whole PR into multiple commits, so it can be more easily reviewed.

I've added more tests for LaTeXStrings and Latexify.

I've also fixed the aspect_ratio after #4231 (comment).

@t-bltg t-bltg force-pushed the tex branch 2 times, most recently from 02ccebc to 6c489c8 Compare July 11, 2022 12:53
@t-bltg t-bltg changed the title PGFPlotsX: fix latexification PGFPlotsX: reworks Jul 11, 2022
Copy link
Member

@BeastyBlacksmith BeastyBlacksmith left a comment

Choose a reason for hiding this comment

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

I am reckognizing that code format is a highly opinionated topic, but some changes here (that I marked with ^) are reducing readability and maintainability in favor of terseness.

In general I try to write code in a way that there is mostly happening one thing at a time on one line and that you have to change few things if you need to extend or refactor the code.

src/backends/pgfplotsx.jl Show resolved Hide resolved
src/backends/pgfplotsx.jl Show resolved Hide resolved
src/backends/pgfplotsx.jl Show resolved Hide resolved
src/backends/pgfplotsx.jl Outdated Show resolved Hide resolved
src/backends/pgfplotsx.jl Show resolved Hide resolved
src/backends/pgfplotsx.jl Show resolved Hide resolved
src/backends/pgfplotsx.jl Show resolved Hide resolved
src/backends/pgfplotsx.jl Show resolved Hide resolved
src/backends/pgfplotsx.jl Show resolved Hide resolved
src/backends/pgfplotsx.jl Show resolved Hide resolved
@t-bltg
Copy link
Member Author

t-bltg commented Jul 12, 2022

I am reckognizing that code format is a highly opinionated topic, but some changes here (that I marked with ^) are reducing readability and maintainability in favor of terseness.

Thanks for the review. I've tried to answer all the comments, explained why some are IMO justified and rebased the PR to remove the modifications on which we agreed are unjustified or not needed.

@t-bltg t-bltg force-pushed the tex branch 3 times, most recently from afc2039 to d98e635 Compare July 12, 2022 18:42
@t-bltg t-bltg force-pushed the tex branch 3 times, most recently from 6f20e7f to dcee6db Compare July 14, 2022 09:00
@t-bltg
Copy link
Member Author

t-bltg commented Jul 22, 2022

I am going to merge the PR when CI passes, unless something new is brought to the table.

@t-bltg t-bltg merged commit 26441f6 into JuliaPlots:master Jul 24, 2022
@t-bltg t-bltg deleted the tex branch July 24, 2022 10:53
@t-bltg t-bltg mentioned this pull request Jul 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] LaTeX error in PGFPlotsX backend. Unicode error with PGFPlotsX [BUG]
2 participants