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 getobj for parameterized callable objects #142

Merged
merged 3 commits into from
Jun 19, 2023

Conversation

lgoettgens
Copy link
Collaborator

Fixes #49 and fixes #141.

The main change is the unwrapping of UnionAlls in getobj. While looking through it, I furthermore used it in the piracy tests as well (where until now the function was basically inlined). And in getobj the deprecated function tuple_type_head was used, the alternative is fieldtype(-, 1). Both changes have been tested in julia 1.0, 1.6 and 1.9.

The MWE in #49 (comment) by @mtsch for julia 1.0 (Aqua.test_ambiguities([MWE, Base, Core], exclude=[convert])) reports precisely the same 52 ambiguities (up to order) as with the current master, but there are no errors.

The much smaller MWE in #49 (comment) by @fingolfin for julia 1.9 no longer fails.

Please check if this fixes it for you as well.

@lgoettgens lgoettgens changed the title Fix getobj Fix getobj for parameterized callable objects Jun 19, 2023
@lgoettgens
Copy link
Collaborator Author

I will try to construct a test for this without introducing an external test dependency.

@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Merging #142 (b18fd2e) into master (03b678b) will increase coverage by 11.08%.
The diff coverage is n/a.

❗ Current head b18fd2e differs from pull request most recent head ca16eb8. Consider uploading reports for the commit ca16eb8 to get more accurate results

@@             Coverage Diff             @@
##           master     #142       +/-   ##
===========================================
+ Coverage   65.69%   76.78%   +11.08%     
===========================================
  Files          11       11               
  Lines         548      715      +167     
===========================================
+ Hits          360      549      +189     
+ Misses        188      166       -22     
Flag Coverage Δ
unittests 76.78% <ø> (+11.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lgoettgens
Copy link
Collaborator Author

For the added testcase in the style of the method erroring #49 (comment), one can now see the error in the CI logs (e.g. https://github.com/JuliaTesting/Aqua.jl/actions/runs/5313233395/jobs/9618834173?pr=142#step:4:139). Unfortunately, due to the implementation of test_ambiguities, we are not able to test for their absence. I'll try to circumvent that somehow.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@fingolfin fingolfin merged commit ed9cb95 into JuliaTesting:master Jun 19, 2023
@lgoettgens lgoettgens deleted the lg/UnionAll branch June 20, 2023 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants