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

TypePiracy: treat foreign type as own type #138

Closed
lgoettgens opened this issue Jun 16, 2023 · 3 comments · Fixed by #140
Closed

TypePiracy: treat foreign type as own type #138

lgoettgens opened this issue Jun 16, 2023 · 3 comments · Fixed by #140

Comments

@lgoettgens
Copy link
Collaborator

Originally brought up in oscar-system/GAP.jl#889 (comment).

The julia docu explains some scenario, where one would have a good reason to do type piracy, e. g. when splitting a C wrapper into a lightweight jll defining the types and another julia package implementing higher level functionality, see https://docs.julialang.org/en/v1/manual/style-guide/#Avoid-type-piracy-1 (last paragraph).

I would thus propose to add a kwarg to the piracy test with a list of types that should be treated as own types even if they are foreign.
Most other Aqua test cases have some kind of ignore kwarg, so I see no point in not having something similar for test_piracy.

@devmotion
Copy link

It would also be useful to be able to specify that a function is owned by a different package: For instance, StatsAPI defines function stumps that are officially declared to be owned by StatsBase - but currently Aqua flags their default implementation for generic types (defined in Base) as type piracy (JuliaStats/StatsBase.jl#866).

@lgoettgens
Copy link
Collaborator Author

So a solution to you would be to split up my proposed kwarg treat_as_own into two different ones: one for types that works as proposed in #140, and one for functions, which just doesn't check type piracy for the functions given there.
Is that what you have in mind?

And I would like to hear the opinion of @fingolfin on this - If there is nothing I have missed, I will update my pr #140 to include your idea as well.

@devmotion
Copy link

Yes, I think such a generalization would fix my use case in StatsBase as well.

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 a pull request may close this issue.

2 participants