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

Removing leaves export #332

Closed
theogf opened this issue Sep 7, 2023 · 5 comments · Fixed by #333
Closed

Removing leaves export #332

theogf opened this issue Sep 7, 2023 · 5 comments · Fixed by #333

Comments

@theogf
Copy link

theogf commented Sep 7, 2023

It is currently quite inconvenient that BenchmarkTools.jl export the leaves function as it is a very common name and tend to clash with a lot of other packages containing Trees.

Would you consider a (breaking) PR which would remove this specific export?

@gdalle
Copy link
Collaborator

gdalle commented Sep 7, 2023

We're heading towards a 2.0 release so it's now or never. Can you maybe check the history of the repo to see whether they list good reasons for the export (issues, PRs, commits)?

@theogf
Copy link
Author

theogf commented Sep 7, 2023

I could not find any discussion on it (PR or issues). It has a very clear use (iteration of a BenchmarkGroup), I guess the name clash was just never really considered.

Another option would be to depend on AbstractTrees.jl but that's a very unnecessary dependency.

@gdalle gdalle added this to the v2.0 milestone Sep 18, 2023
@lucaferranti
Copy link
Contributor

I also was hit by this a few times and I think it would make sense not to export it.

It's good to mention also that atm the function does not have a docstring and is not included in the References section of the docs. Based on the short example in the manual it looks like something most end users would not need? (At least I dont recall needing it).

@gdalle
Copy link
Collaborator

gdalle commented Sep 19, 2023

Wanna make a PR? Should be a quick review

@lucaferranti
Copy link
Contributor

Can open one tomorrow morning if nobody ninjas me by then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants