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

Change deprecated textsize to fontsize #21

Merged
merged 2 commits into from
Jan 19, 2023
Merged

Conversation

andrewwinters5000
Copy link
Member

Newer releases of Makie have replaced textsize everywhere in favor of fontsize.

Closes #20

ranocha
ranocha previously approved these changes Jan 17, 2023
Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

Sadly, we can't have compat bounds on optional dependencies right now. Thus, this will break the plotting for people using older versions of Makie, but that's the best what we can do right now 🤷

@sloede
Copy link
Member

sloede commented Jan 17, 2023

Is there any (sane) way that we can use to warn people about it for now? Eg, in the require clause of Makie, check if the new entity exists in the Makie module and if not, print a warning?

@andrewwinters5000
Copy link
Member Author

Thus, this will break the plotting for people using older versions of Makie

On my machine (that uses an older Makie 0.7) everything still worked.

@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Merging #21 (330b4bd) into main (c7f9ca8) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #21   +/-   ##
=======================================
  Coverage   98.85%   98.85%           
=======================================
  Files          20       20           
  Lines        1838     1838           
=======================================
  Hits         1817     1817           
  Misses         21       21           
Flag Coverage Δ
unittests 98.85% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/HOHQMesh.jl 100.00% <ø> (ø)
src/Viz/VizProject.jl 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ranocha
Copy link
Member

ranocha commented Jan 17, 2023

Thus, this will break the plotting for people using older versions of Makie

On my machine (that uses an older Makie 0.7) everything still worked.

That's great 👍

@ranocha
Copy link
Member

ranocha commented Jan 17, 2023

Is there any (sane) way that we can use to warn people about it for now? Eg, in the require clause of Makie, check if the new entity exists in the Makie module and if not, print a warning?

Not really (unless you want to fiddle with some internal Pkg stuff that may change any time). It's a keyword argument, so we can' just check something like isdefined(...).

@coveralls
Copy link

coveralls commented Jan 17, 2023

Coverage Status

Coverage: 98.857% (+0.0%) from 98.857% when pulling 330b4bd on update-plot-label into c7f9ca8 on main.

@sloede
Copy link
Member

sloede commented Jan 17, 2023

I was thinking about something very simple like

if isdefined(Makie, :to_textsize)
  @warn "You seem to be using an older version of Makie (< v0.19). Not all plotting functions might work."
end

But I have no strong opinion on this 🤷

@andrewwinters5000
Copy link
Member Author

I was thinking about something very simple like

if isdefined(Makie, :to_textsize)
  @warn "You seem to be using an older version of Makie (< v0.19). Not all plotting functions might work."
end

Would this go into the function __init__() of HOHQMesh.jl?

@sloede
Copy link
Member

sloede commented Jan 17, 2023

I was thinking about something very simple like

if isdefined(Makie, :to_textsize)
  @warn "You seem to be using an older version of Makie (< v0.19). Not all plotting functions might work."
end

Would this go into the function __init__() of HOHQMesh.jl?

Yes, after this line (untested):

using .Makie

@andrewwinters5000
Copy link
Member Author

I looked into the Makie.jl source a bit and I am surprised that I encounter no issues.
From my end a warning is not necessary because nothing changed in the output on my machine with an old version of Makie. I was wrong before, however, my GLMakie is v0.7 whereas my Makie is v0.18.0.

Can someone else with an older version of Makie verify that one can swap the keyword argument textsize to fontsize and nothing changes?

@andrewwinters5000
Copy link
Member Author

Yes, after this line (untested):

Okay, that works. I get thrown a warning now on my machine but all the labels are still working properly.

DavidAKopriva
DavidAKopriva previously approved these changes Jan 17, 2023
@andrewwinters5000
Copy link
Member Author

I have lost the thread here, should this merge as is or do we want to add the warning message?

@DavidAKopriva
Copy link
Collaborator

Doesn't hurt to add the warning message. And it would serve as a template for future changes and warnings that might/will arise when they change things again.

@andrewwinters5000 andrewwinters5000 merged commit 35e02a2 into main Jan 19, 2023
@andrewwinters5000 andrewwinters5000 deleted the update-plot-label branch January 19, 2023 06:29
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.

Error with latest Makie
5 participants