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

fixed inappropriate deprecation warnings when checking for sub-modules #41

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

ExpandingMan
Copy link
Contributor

This fixes #37 and #40.

I don't believe this can break anything since the function in question is used for checking for sub-modules which are unaffected by this check.

I think it's important to fix this since testing is precisely the context in which you'd want to check for deprecations so it's profoundly unhelpful to have the testing module itself throw bogus deprecation warnings.

@ExpandingMan
Copy link
Contributor Author

I think tests are breaking due to fragile RNG. I'm going to try to fix it but I'm not entirely sure I grok what was being tested for yet...

@ExpandingMan
Copy link
Contributor Author

Hm... so it seems that some RNG tests fail for me locally but succeed on CI/CD. Suggest we open an issue and leave it for another PR.

@ExpandingMan
Copy link
Contributor Author

bump

1 similar comment
@ExpandingMan
Copy link
Contributor Author

bump

@DilumAluthge
Copy link
Member

Who would be the right person to review this? @rfourquet, probably?

@rfourquet
Copy link
Collaborator

Thanks a lot @ExpandingMan, and very sorry for the delay, had a hard time keeping up with Julia development recently.
I will merge as it seems indeed harmless for submodules to function properly, although I don't really understand why the warnings were triggered (and I couldn't reproduce the issue, though I didn't try hard TBH!) So just to try to understand slightly better, would you be willing to check whether instead of your fix, you used getfield instead of eval ? (getfield is anyway probably generally better)

function submodules(m::Module)
    symbols = getfield.(Ref(m), filter!(y -> isdefined(m, y), names(m, all=true)))
    filter!(x -> issubmodule(m, x), symbols)
end

@rfourquet
Copy link
Collaborator

Ok, I managed to reproduce the problem thanks to #43, and using getfield instead of eval didn't help.

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.

mysterious deprecation warnings
3 participants