Allow check_*_public functions to opt-in dependencies to check#134
Allow check_*_public functions to opt-in dependencies to check#134ericphanson merged 5 commits intomainfrom
check_*_public functions to opt-in dependencies to check#134Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a from keyword parameter to check_all_qualified_accesses_are_public and check_all_explicit_imports_are_public functions, allowing users to restrict public name checks to only specific modules. Instead of checking all qualified accesses or imports, users can now target only accesses/imports from particular modules (e.g., DataFrames, LinearAlgebra).
Key changes:
- Added
from::Tuple=()parameter to both check functions - Implemented filtering logic to keep only items from specified modules when
fromis non-empty - Added documentation and test cases for the new functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/checks.jl | Added from parameter and filtering logic to both check_all_qualified_accesses_are_public and check_all_explicit_imports_are_public functions, plus documentation |
| test/runtests.jl | Added test cases verifying the from parameter filters to only specified modules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/checks.jl
Outdated
| for from_mod in from | ||
| filter!(problematic) do row | ||
| return row.accessing_from == from_mod | ||
| end | ||
| end |
There was a problem hiding this comment.
this doesn't quite look right, isn't this the intersection of from, not the union?
There was a problem hiding this comment.
3 min old PR and I'm scooped by a robot :(. at least reassuring we both noticed it.
edit: though its explanation of the issue does not seem to be quite correct
There was a problem hiding this comment.
oops. Thanks! Fixed
There was a problem hiding this comment.
(also copilot wrote this code, so it should really have a word with itself)
|
Is it okay to allow this to work with ? And is there a nice way to do that? (Other than something like That way for my use-case I could write a test helper that hardcodes the list, like |
|
We don't allow symbols for modules anywhere else, so I'd rather not. In your tests though you could add a line to get the modules from a symbol list (with eval I guess) when they're loaded. |
|
if you pass ? i think probably yes, so we would need to see if |
|
oh yes i meant to ask what behaviour you wanted for submodules.
makes sense to me -- will look into it |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ericphanson
left a comment
There was a problem hiding this comment.
LGTM. Bump the minor version for a feature release
src/checks.jl
Outdated
| check_all_qualified_accesses_are_public(mod::Module, file=pathof(mod); ignore::Tuple=(), | ||
| skip::$(TUPLE_MODULE_PAIRS)=(Base => Core,), | ||
| allow_internal_accesses=true) | ||
| from::Tuple=(), allow_internal_accesses=true) |
There was a problem hiding this comment.
Actually, I don't like using the empty set as a sentinel here, because it has the wrong semantics; empty set should not check anything. That seems pedantic at first but from could be programmatically generated and happen to come up empty in some situation where you don't want to check anything. SO let's make the default nothing, then change the check from isempty to isnothing. Then whenever from is not nothing, we will check it. We will also allow vectors and stuff, not just tuples.
| from::Tuple=(), allow_internal_accesses=true) | |
| from=nothing, allow_internal_accesses=true) |
There was a problem hiding this comment.
oo, yeah that's nicer. done.
ericphanson
left a comment
There was a problem hiding this comment.
actually, I do want one more change for the defaults, see comment
|
Thanks a lot, Eric! <3 |
Adds a new keyword
from(name TDB) to the functionscheck_all_qualified_accesses_are_publicandcheck_all_explicit_imports_are_public, which allows specifying which dependencies we want to include in the check.The use-case is migrating a codebase to use only public APIs, and wanting to incrementally lock-in the progress made.
E.g. suppose
MyPackagedepends on many packages, includingLinearAlgebraandDataStructures, and uses non-public functions from all of them. First we'd like to updateMyPackageto use only the public API of LinearAlgebra.jl, then add the test:then update
MyPackageto only use public API of DataStructures.jl and update the test to be:and so on until finally the test can become simply
Right now incremental test coverage is possible on an "opt-out" basis, using
ignoreand/orskipas appropriate, but this PR adds the ability to test on an "opt-in" basis.Our actual use-case is slightly more involved: we have N internal packages
MyPkg1,MyPkg2, ...,MyPkgN, with dependencies between them, and many packages don't even declare a public API (noexportorpublic), so we'd like to go through and (i) giveMyPkg1a public API, and (ii) update allMyPkgXpackages to only use that API and lock that in, which is why we want to "opt-in" a package at a time