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

Update MIME type for show #1322

Merged
merged 2 commits into from
Oct 25, 2019
Merged

Update MIME type for show #1322

merged 2 commits into from
Oct 25, 2019

Conversation

jjerphan
Copy link
Contributor

Fixes #1316

Contributor checklist:

  • I've updated the documentation to reflect these changes: no change required
  • I've added an entry to NEWS.md
  • I've added and/or updated the unit tests: no need to.
  • I've run the regression tests: all the passing test on master pass on this branch
  • I've squash'ed or fixup'ed junk commits with git-rebase: one commit PR
  • I've built the docs and confirmed these changes don't cause new errors

Proposed changes

  • Resolves the deprecation

@tlnagy
Copy link
Member

tlnagy commented Sep 24, 2019

Thanks for the PR! Do you have a reference for when this change was made in Juno? It would be best to put a lower bound in the [compat] section of Project.toml for this change

@jjerphan
Copy link
Contributor Author

@tlnagy: I don't know at all, I'll try to find it in Juno's history. :)

@jjerphan
Copy link
Contributor Author

jjerphan commented Sep 25, 2019

It comes from this commit in Atom.jl.

It seems that v0.7.9 is the lower bound — I don't know whether I should add Atom in Project.toml or find the appropriate version of Juno that uses this version of Atom.

@tlnagy
Copy link
Member

tlnagy commented Sep 25, 2019

Please don't add Atom.jl to Project.toml.

find the appropriate version of Juno that uses this version of Atom.

This is the correct way. If compatible Juno versions don't have a hard lower bound against the Atom, I would file an issue with them since this would need to get fixed on their end.

@jjerphan
Copy link
Contributor Author

jjerphan commented Sep 26, 2019

It looks like it's Atom.jl that depends on Juno.jl — see here

@bjarthur
Copy link
Member

@jjerphan thanks for taking the time to submit this PR!

i would suggest keeping both methods, so that Gadfly support both old and new versions of Atom. so make a Union of those mime types: m::Union{MIME"application/juno+plotpane, MIME"application/prs.juno.plotpane+html"}"

@codecov-io
Copy link

codecov-io commented Oct 20, 2019

Codecov Report

Merging #1322 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1322      +/-   ##
==========================================
- Coverage    90.1%   90.02%   -0.09%     
==========================================
  Files          36       36              
  Lines        3953     3941      -12     
==========================================
- Hits         3562     3548      -14     
- Misses        391      393       +2
Impacted Files Coverage Δ
src/Gadfly.jl 77.58% <ø> (-1.01%) ⬇️
src/theme.jl 69.49% <0%> (-1.7%) ⬇️
src/mapping.jl 84.46% <0%> (-0.98%) ⬇️
src/geom/errorbar.jl 92.3% <0%> (+0.41%) ⬆️
src/dataframes.jl 97.43% <0%> (+6.86%) ⬆️

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 0eed988...8132ee4. Read the comment docs.

@jjerphan
Copy link
Contributor Author

@bjarthur: I've performed this latter small change.

@tlnagy: just a little follow up on my last comment: what should be done regarding the compatibility with Atom.jl?

@bjarthur
Copy link
Member

i don't think anything needs to be done with [compat]. think this is fine as is and ready to go.

@tlnagy shall we merge? just need to squash the two commits when we do.

@tlnagy
Copy link
Member

tlnagy commented Oct 25, 2019

Agreed.

@tlnagy tlnagy merged commit 56c0112 into GiovineItalia:master Oct 25, 2019
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.

MIME-type when plotting in Atom/Juno
4 participants