Skip to content

Conversation

@t-bltg
Copy link
Member

@t-bltg t-bltg commented Oct 14, 2022

  • drop Contour, LaTeXStrings and Latexify hard dependencies, since they are only used by pgfplotsx backend;
  • resolve methods ambiguities detected by Aqua;
  • fix pie aspect_ratio bug (leading to a Gaston warning);
  • syntax simplifications;
  • cleanup dead code;
  • improve coverage.

@t-bltg t-bltg added enhancement improving existing functionality ci continuous integration labels Oct 14, 2022
@t-bltg t-bltg marked this pull request as draft October 14, 2022 15:35
@BeastyBlacksmith
Copy link
Member

  • drop Contour, LatexStrings and Latexify hard dependencies, since they are only used by pgfplotsx backend;

What is the benefit of that? For one you loose the ability of specifying compat entries and its also a breaking change.

@t-bltg
Copy link
Member Author

t-bltg commented Oct 14, 2022

What is the benefit of that

Well, pgfplotsx is an optional backend, so should Contour and Latexify be optional dependencies for Plots. Maybe we should create a glue package for PGFPlotsX ?
LaTeXStrings is already a Latexify dependency, so it is a bit different, we can cheat by using Latexify.LaTeXStrings from within Plots.

Compatibility is still checked in here.

This ambiguity was spotted by Aqua tests as stale dependencies (since only triggered by @require).

@t-bltg t-bltg changed the title add quality test (Aqua) add quality test using Aqua Oct 14, 2022
@t-bltg t-bltg force-pushed the aqua branch 5 times, most recently from 15dfc07 to ec9d69b Compare October 14, 2022 18:11
@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Attention: Patch coverage is 81.28655% with 32 lines in your changes missing coverage. Please review.

Project coverage is 82.22%. Comparing base (c5fa70c) to head (1b5e4e0).
Report is 425 commits behind head on master.

Files with missing lines Patch % Lines
src/recipes.jl 69.56% 7 Missing ⚠️
src/utils.jl 81.08% 7 Missing ⚠️
src/axes.jl 86.48% 5 Missing ⚠️
src/backends/hdf5.jl 42.85% 4 Missing ⚠️
src/backends/gaston.jl 83.33% 3 Missing ⚠️
RecipesBase/src/RecipesBase.jl 91.30% 2 Missing ⚠️
src/args.jl 80.00% 2 Missing ⚠️
src/backends.jl 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4456      +/-   ##
==========================================
+ Coverage   80.83%   82.22%   +1.39%     
==========================================
  Files          40       40              
  Lines        8185     8077     -108     
==========================================
+ Hits         6616     6641      +25     
+ Misses       1569     1436     -133     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@t-bltg t-bltg added coverage and removed enhancement improving existing functionality labels Oct 15, 2022
@BeastyBlacksmith
Copy link
Member

BeastyBlacksmith commented Oct 17, 2022

Well, pgfplotsx is an optional backend, so should Contour and Latexify be optional dependencies for Plots. Maybe we should create a glue package for PGFPlotsX ?

These are all packages where its not worth the hassle to put that much work into it over just shipping them. In principle I agree it would be nice to have a better mechanism for optional dependencies, especially because of compat. But that would need to land in Pkg first.

For reference:

julia> @time_imports using Contour
      4.6 ms  Contour
julia> @time_imports using Latexify
      0.6 ms  Requires
      0.9 ms  LaTeXStrings
     27.5 ms  MacroTools
      0.4 ms  Formatting
      4.6 ms  OrderedCollections
     32.9 ms  Latexify 41.86% compilation time (7% recompilation)

@t-bltg
Copy link
Member Author

t-bltg commented Oct 17, 2022

Thanks, I've removed the breaking PGFPlotsX changes, which should optionally be included in a future Plots 2.0.

@t-bltg t-bltg marked this pull request as ready for review October 17, 2022 09:01
@t-bltg t-bltg force-pushed the aqua branch 3 times, most recently from 1ec0666 to ace5379 Compare October 17, 2022 09:58
@t-bltg t-bltg merged commit 4366f8a into JuliaPlots:master Oct 17, 2022
@t-bltg t-bltg deleted the aqua branch October 17, 2022 13:24
@BeastyBlacksmith
Copy link
Member

In principle I agree it would be nice to have a better mechanism for optional dependencies, especially because of compat. But that would need to land in Pkg first.

Relevant PR to track: JuliaLang/Pkg.jl#3216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants