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 deprecation warnings #70

Merged
merged 1 commit into from
Apr 23, 2017
Merged

Conversation

KristofferC
Copy link
Contributor

Wrapping ALL of PGFPlots is likely far away, this provides an escape hatch to inject arbitrary options into an axis:

image

This PR also fixes the deprecation warnings for 0.6.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1955e5e on KristofferC:kc/customOptions into ** on sisl:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e1a8f7e on KristofferC:kc/customOptions into ** on sisl:master**.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e1a8f7e on KristofferC:kc/customOptions into ** on sisl:master**.

@KristofferC
Copy link
Contributor Author

KristofferC commented Apr 20, 2017

Hmm, I wonder if this is the best interface. Sometimes you don't want to give a key value pair but simply a string (like xmajorgrids). Perhaps the dict should just be a Vector{String} and one would have to pass things like customOptions = ["cycle list name = exotic"]

@KristofferC
Copy link
Contributor Author

It could perhaps also be nice to be able to give a customOptions to the different plot objects (in the same way as is done for the axis here).

@tawheeler
Copy link
Member

Thank you for writing this!
One thing that has bugged me with PGFPlots is that it doesn't store the data in a data structure resembling the hierarchical property trees that LaTeX code is written in. We currently typically use strings like style="width=8cm, black, opacity=0.5, legend style={at={(0.03,0.97)}}" to represent what is really more like a json object:

{
    "width": "8cm",
    "color": "black",
    "opacity": 0.5,
    "legend style": {
        "at": "{(0.3,0.97)}",
    }
}

There are some differences - "black" for instance can be passed in as a value without having an associated key name.
It would be nice if PGFPlots was changed to convert passed in parameters to such a form that could be queried, etc. Right now it isn't easy to tell if a particular value has been or has not been set.

Definitely want to hear what @mykelk has to say.

@mykelk
Copy link
Member

mykelk commented Apr 20, 2017

Arbitrary options can be passed to an axis through the style keyword argument. I think the only functionality this adds is converting a Dict to a string, but I think we can handle that by dispatching on the type passed to style. Right?

It might feel a little strange passing these options in as a string, but I put that support in initially so that we can do anything the latex package supports. One thing that I was thinking of doing was allowing varargs and then converting those key-value pairs. I turns out that Julia's version of varargs even supports non-assignments, like the "black" option that @tawheeler mentions above.

I'd like to know your thoughts. And thank you for your contribution! We'll want to settle these question before merging. (And thanks for the fixes for 0.6.)

@KristofferC
Copy link
Contributor Author

I didn't think about passing it through style, that makes this PR redundant. I can split out the deprecation commits to a separate PR.

@mykelk
Copy link
Member

mykelk commented Apr 20, 2017

Sounds good. It would be great to get those deprecation commits in. Thanks!

@KristofferC KristofferC changed the title Add an escape hatch to add arbitrary options to an axis Fix deprecation warnings Apr 21, 2017
@KristofferC
Copy link
Contributor Author

I've changed this to only fix deprecation warnings in code and notebook

@coveralls
Copy link

coveralls commented Apr 21, 2017

Coverage Status

Changes Unknown when pulling 4eecb93 on KristofferC:kc/customOptions into ** on sisl:master**.

@tawheeler
Copy link
Member

Btw further discussion should be on #69

@mykelk
Copy link
Member

mykelk commented Apr 21, 2017

@tawheeler I'm okay with merging if you are. Go for it if you're good with it.

@tawheeler tawheeler merged commit 150d513 into JuliaTeX:master Apr 23, 2017
@tawheeler
Copy link
Member

Thank you!

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.

4 participants