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

[RecipesBase] Use interpolation instead of qualification to drop dependance on the namespace of RecipesBase users #4559

Merged
merged 9 commits into from Nov 30, 2022

Conversation

LilithHafner
Copy link
Contributor

@LilithHafner LilithHafner commented Nov 28, 2022

We can't assume that folks will have RecipesBase in their namespace.

Fixes #4557

@LilithHafner
Copy link
Contributor Author

CI is already failing after just a few minutes <3

Use interpolation instead of qualification
@LilithHafner LilithHafner changed the title Make tests stricter [RecipesBase] Use interpolation instead of qualification to drop dependance on the namespace of RecipesBase users Nov 28, 2022
@BeastyBlacksmith
Copy link
Member

BeastyBlacksmith commented Nov 28, 2022

We can't expect them to have all those internal functions in the namespace either, I'd say.
The proper fix would be to document that users should at least use

using RecipesBase: RecipesBase, @recipe

@t-bltg
Copy link
Member

t-bltg commented Nov 28, 2022

As Simon suggested, improving the docs is probably the easiest.

With a minor nitpick:

import RecipesBase: RecipesBase, @recipe

@LilithHafner
Copy link
Contributor Author

The original approach (fb77991) should work if JuliaLang/julia#47726 is fixed upstream.

@LilithHafner
Copy link
Contributor Author

Failing that, a slightly more verbose approach should work following this pattern

julia> module NanoRecipesBase
           function apply_recipe end
           macro recipe(x)
               quote
                   let M = $NanoRecipesBase
                       M.apply_recipe() = $x
                   end
               end
           end
       end
Main.NanoRecipesBase

julia> module User
           import Main.NanoRecipesBase: @recipe
           try
               println(NanoRecipesBase)
           catch UndefVarError
               println("Good")
           end
           @recipe 1729
       end
Good
Main.User

julia> NanoRecipesBase.apply_recipe()
1729

@LilithHafner
Copy link
Contributor Author

The proper fix would be to document that users should at least use

As Simon suggested, improving the docs is probably the easiest.

We should only document an unexpected limitation/bug if we can't fix it. This PR should fix it :)

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Base: 90.94% // Head: 90.95% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e9e12d6) compared to base (497cec4).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4559   +/-   ##
=======================================
  Coverage   90.94%   90.95%           
=======================================
  Files          40       40           
  Lines        7800     7802    +2     
=======================================
+ Hits         7094     7096    +2     
  Misses        706      706           
Impacted Files Coverage Δ
RecipesBase/src/RecipesBase.jl 94.21% <100.00%> (ø)
src/examples.jl 97.59% <0.00%> (ø)
src/animation.jl 99.04% <0.00%> (ø)
src/backends/gr.jl 91.65% <0.00%> (ø)
src/utils.jl 92.61% <0.00%> (+0.02%) ⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@t-bltg t-bltg added enhancement improving existing functionality RecipesBase labels Nov 28, 2022
@LilithHafner
Copy link
Contributor Author

The format check gave a lot of diffs. Is there a way to avoid transcribing all those non-functional-changes manually?

@t-bltg
Copy link
Member

t-bltg commented Nov 28, 2022

https://docs.juliaplots.org/latest/contributing/#Write-code,-and-format

$ julia -e 'using JuliaFormatter; format(["src", "test"])'

Copy link
Member

@t-bltg t-bltg left a comment

Choose a reason for hiding this comment

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

Nice.

@t-bltg
Copy link
Member

t-bltg commented Nov 28, 2022

Seems like using GlobalRef triggers a segfault apocalypse here 😆

@LilithHafner
Copy link
Contributor Author

Strange. I guess I'll just revert to 5440348 "fix formatting".

@LilithHafner
Copy link
Contributor Author

I don't understand the GlobalRef segfaults so I asked on Discourse: https://discourse.julialang.org/t/what-is-a-globalref-and-how-i-use-it/90971

@LilithHafner
Copy link
Contributor Author

Thanks @ffevotte from discourse for the latest and best workaround for julia#47726

@t-bltg
Copy link
Member

t-bltg commented Nov 29, 2022

Looks good, thanks for getting to the bottom of this with a cleaner solution. Still intrigued by the GlobalRef segfault though 🤔

@t-bltg t-bltg self-requested a review November 30, 2022 07:51
RecipesBase/test/runtests.jl Outdated Show resolved Hide resolved
Co-authored-by: Simon Christ <SimonChrist@gmx.de>
@LilithHafner
Copy link
Contributor Author

I'm happy with de5a423, but I believe e9e12d6 contains unnecessarily indirect interpolation.

@t-bltg
Copy link
Member

t-bltg commented Nov 30, 2022

I'm happy with de5a423, but I believe e9e12d6 contains unnecessarily indirect interpolation.

Do you imply that those added interpolations have a runtime performance penalty ?

@LilithHafner
Copy link
Contributor Author

Do you implicit that those added interpolations have a runtime performance penalty ?

No. There should not be a runtime penalty.

@LilithHafner
Copy link
Contributor Author

Ultimately, it's just a style/formatting choice. I prefer direct interpolation over module interpolation + qualified reference because it is shorter, but either should work, and y'all are more likely to be maintaining this than me so feel free to pick whatever style you prefer.

@t-bltg t-bltg merged commit 3dd7ad2 into JuliaPlots:master Nov 30, 2022
@t-bltg
Copy link
Member

t-bltg commented Nov 30, 2022

We disagree on this one, and I explained my position in #4559 (comment).

Thanks for the contribution and the detailed analysis here, always useful to learn about the internals !

@LilithHafner LilithHafner deleted the patch-1 branch November 30, 2022 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improving existing functionality RecipesBase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] using RecipesBase: @recipe is broken
3 participants