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

fix(styles): platform evaluation for Linux was incorrect #2188

Merged
merged 2 commits into from
May 9, 2024

Conversation

jdhughes-usgs
Copy link
Contributor

No description provided.

@jdhughes-usgs jdhughes-usgs requested a review from wpbonelli May 8, 2024 18:37
@jdhughes-usgs
Copy link
Contributor Author

@wpbonelli I am surprised we haven't caught the issue with flopy.plot.styles on Linux before. platform.platform was being compared to "linux", which as far as I can see is always "Linux". Am I missing something?

@mwtoews
Copy link
Contributor

mwtoews commented May 8, 2024

Another "fix" would be to remove the extra path logic using two style files and just make one style file with a bunch of alternative names for "font.sans-serif". I.e., remove usgsmap_linux.mplstyle and usgsplot_linux.mplstyle and with the two remaining style files, define:

font.sans-serif: Arial, Liberation Sans

and maybe add other possible sans-serif fonts to choose. See this file for default styles.

Copy link
Member

@wpbonelli wpbonelli left a comment

Choose a reason for hiding this comment

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

I'm surprised we haven't caught this too. I like your idea @mwtoews, maybe we come back to that

@jdhughes-usgs
Copy link
Contributor Author

Another "fix" would be to remove the extra path logic using two style files and just make one style file with a bunch of alternative names for "font.sans-serif". I.e., remove usgsmap_linux.mplstyle and usgsplot_linux.mplstyle and with the two remaining style files, define:

font.sans-serif: Arial, Liberation Sans

and maybe add other possible sans-serif fonts to choose. See this file for default styles.

This is a better approach since it simplifies things. Thanks for the suggestion @mwtoews

Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.6%. Comparing base (029a4e1) to head (c4388e2).
Report is 17 commits behind head on develop.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #2188     +/-   ##
=========================================
+ Coverage     68.3%   70.6%   +2.2%     
=========================================
  Files          260     261      +1     
  Lines        57947   58080    +133     
=========================================
+ Hits         39611   41022   +1411     
+ Misses       18336   17058   -1278     
Files Coverage Δ
flopy/plot/styles.py 27.2% <ø> (-0.2%) ⬇️

... and 141 files with indirect coverage changes

@jdhughes-usgs jdhughes-usgs merged commit acfd0d3 into modflowpy:develop May 9, 2024
24 checks passed
@jdhughes-usgs jdhughes-usgs deleted the fix-styles branch May 10, 2024 14:11
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.

3 participants