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 recipe docs #1448

Merged
merged 2 commits into from Dec 10, 2021
Merged

Fix recipe docs #1448

merged 2 commits into from Dec 10, 2021

Conversation

jonas-schulze
Copy link
Contributor

No description provided.

@jonas-schulze jonas-schulze marked this pull request as draft November 16, 2021 09:04
@jonas-schulze jonas-schulze changed the title Fix code block Fix recipe docs Nov 16, 2021
`argument_names(::Type{<: MyPlot}) = (:x, :y, :z)`
```julia
argument_names(::Type{<: MyPlot}) = (:x, :y, :z)
```

This is optional but it will allow the use of `plot_object[:x]` to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Later on in the stocks example, the arguments are fetched like plot_object.x. Is both supported or should the above be changed to match the example?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer plot.x and would like to deprecate plot[:x] at some point, or change its meaning...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I change this in the docs to bridge the gap while you are working on the overhaul? I couldn't find where plot.x get's wired into getproperty, so I didn't feel comfortable doing the change right away. Here are some other occurences:

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, somehow missed your comment... Yeah I think that's a good idea!

Its here: https://github.com/JuliaPlots/Makie.jl/blob/78820311c9b45347f0937caf8b31a25bbc27dd73/MakieCore/src/attributes.jl#L189

And then getproperty uses getindex here:
https://github.com/JuliaPlots/Makie.jl/blob/78820311c9b45347f0937caf8b31a25bbc27dd73/MakieCore/src/attributes.jl#L83
(Not very straightforward to find, especially with the Transformable supertype)

@jonas-schulze jonas-schulze marked this pull request as ready for review November 16, 2021 10:13
@jonas-schulze
Copy link
Contributor Author

Just found out about #771 and friends. So maybe this isn't needed anymore?

@SimonDanisch SimonDanisch mentioned this pull request Dec 8, 2021
13 tasks
@SimonDanisch SimonDanisch merged commit f283e55 into MakieOrg:master Dec 10, 2021
@ghost ghost deleted a comment Jan 3, 2022
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.

None yet

2 participants