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 tests on Julia 1.10+ #168

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Fix tests on Julia 1.10+ #168

merged 1 commit into from
Aug 13, 2024

Conversation

mortenpi
Copy link
Member

@mortenpi mortenpi commented Aug 13, 2024

Alternative to #167. It looks like there has been a bugfix in 1.10, and the auto-generated methods when you have positional arguments with default values:

g(x = 1, y = 2, z = 3; kwargs...) = x

On 1.9:

julia> methods(M.g)
# 4 methods for generic function "g" from Main.M:
 [1] g()
     @ ~/juliadocs/clones/DocStringExtensions.jl/test/TestModule/M.jl:7
 [2] g(x)
     @ ~/juliadocs/clones/DocStringExtensions.jl/test/TestModule/M.jl:7
 [3] g(x, y)
     @ ~/juliadocs/clones/DocStringExtensions.jl/test/TestModule/M.jl:7
 [4] g(x, y, z; kwargs...)
     @ ~/juliadocs/clones/DocStringExtensions.jl/test/TestModule/M.jl:7

vs on 1.10:

julia> methods(M.g)
# 4 methods for generic function "g" from Main.M:
 [1] g(; ...)
     @ ~/juliadocs/clones/DocStringExtensions.jl/test/TestModule/M.jl:7
 [2] g(x, y, z; kwargs...)
     @ ~/juliadocs/clones/DocStringExtensions.jl/test/TestModule/M.jl:7
 [3] g(x, y; ...)
     @ ~/juliadocs/clones/DocStringExtensions.jl/test/TestModule/M.jl:7
 [4] g(x; ...)
     @ ~/juliadocs/clones/DocStringExtensions.jl/test/TestModule/M.jl:7

So I would argue that the new printing is more correct actually, and so we should instead make the tests version-conditional.

It wasn't obvious to me if there's any way to support the new printing on older Julia versions -- it looks like the information about the kwargs is just missing from the method metadata. It would be good to know which PR in Julia changed this though.

Also makes the CI to run weekly on a schedule, so that we'd maybe catch nightly regressions earlier.

Comment on lines +9 to +10
schedule:
- cron: '0 0 * * 1' # runs 00:00 UTC on every Monday
Copy link
Member

Choose a reason for hiding this comment

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

Does a failure here notify anyone within the org @mortenpi?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I think it will notify whoever is the author of the last commit, which might not be ideal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it looks like it's the person who changes the cron line:

Notifications for scheduled workflows are sent to the user who last modified the cron syntax in the workflow file. For more information, see "Notifications for workflow runs."

Which would be me right now, which seems fine.

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/configuring-notifications#github-actions-notification-options I think it will notify the people that actively subscribe to e.g. failed runs? I don't think that is turned on by default.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'm subscribed to failures so between us it should be sufficient.

@mortenpi mortenpi merged commit f65720c into master Aug 13, 2024
10 checks passed
@mortenpi mortenpi deleted the mp/fix-1.10-tests branch August 13, 2024 07:58
This was referenced Aug 13, 2024
@ChrisRackauckas
Copy link

This did not remove the libgit2 dep?

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.

4 participants