feat!: test imports and use of private APIs through ExplicitImports.jl#369
feat!: test imports and use of private APIs through ExplicitImports.jl#369gdalle wants to merge 7 commits intoJuliaTesting:masterfrom
Conversation
|
I personally think See also: https://github.com/JuliaTesting/ExplicitImports.jl?tab=readme-ov-file#why-not |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #369 +/- ##
==========================================
- Coverage 86.77% 86.30% -0.47%
==========================================
Files 11 12 +1
Lines 514 511 -3
==========================================
- Hits 446 441 -5
- Misses 68 70 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I agree that explicit imports are more controversial, but they're generally a good idea for package maintenance, while allowing readers to know upfront what your code depends on. Style is a part of code quality after all, and while implicit imports might be more convenient, I don't think anyone would say they are "higher quality". Presumably, the v1 release of Aqua will break a lot of tests anyway, and people will have to turn some of the checks off. If we leave one check deactivated, people won't remember to activate it on their own (for instance, I don't think that many people are using the |
|
Very much in favor of all the checks being enabled by default on the upcoming breaking release. That is the point of Aqua and the "proper" way to not be surprised by breaking new checks is to put a compat bound on Aqua and have dependabot update it. Tangential ecosystem questions related to this change:
|
|
My concern is not the breakage, it's that I think Aqua serves as a set of recommendations and I don't think it's the right recommendation to make in general:
I think the "semver hole" of two packages exporting clashing symbols is a real problem, and I can't think of a solution besides qualified access or explicit imports, but I think nudging developers too hard towards explicit imports is not currently the right answer, because the tooling is immature so it'll be a bad experience. |
fingolfin
left a comment
There was a problem hiding this comment.
Thanks for your contribution. However I am very sceptical about this change. IMHO having ExplicitImports.test_explicit_imports as a required check goes a step to far. @ericphanson puts it right: think the tooling is not good enough.
Using ExplicitImports is every unergonomic as the list of "TODO: fix instead of ignoring shows". The fact that you left that as a TODO instead of "just fixing it" is IMHO telling.
Also, since apparently ExplicitImports.test_explicit_imports exists, it doesn't seem to me we are adding much value anyway. If someone wants to use ExplicitImports but personally I wouldn't want to force this on everyone using Aqua at this point. I certainly would not want it for my packages.
| links = InterLinks( | ||
| "ExplicitImports" => "https://juliatesting.github.io/ExplicitImports.jl/stable/", | ||
| ) | ||
|
|
There was a problem hiding this comment.
What is this needed for / what does it gain us?
There was a problem hiding this comment.
It allows for a direct link to the docstring of ExplicitImports.test_explicit_imports, so that we don't have to separately maintain a list of its keyword arguments
| """ | ||
| test_explicit_imports(m::Module; kwargs...) | ||
|
|
||
| Run the various tests provided by the package [ExplicitImports.jl](https://github.com/ericphanson/ExplicitImports.jl). |
There was a problem hiding this comment.
I don't understand: if ExplicitImports.test_explicit_imports already exists, why do we need to add this to Aqua at all?
There was a problem hiding this comment.
To have everything in one place instead of requiring users to combine several packages when testing code quality
There was a problem hiding this comment.
But we are missing JET report_package, SnoopeCompile invalidation tests, Runic (or JuliaFormatter???) code formatting.
Sarcasm aside, I don't agree with this being a useful goal.
There was a problem hiding this comment.
You think this is sarcasm but I actually think these would be other legitimate additions to Aqua
There was a problem hiding this comment.
The main question is what the community wants, because you and I are obviously on opposite sides ^^
There was a problem hiding this comment.
The main question is what the community wants
Is it, though? It sure is useful what "the community" wants, but it is also exceedingly difficult to actually find that out.
In particular don't find 15 likes on an issue convincing: first off, those are not really high numbers (relative to the size of the Julia community). Moreover you don't see dislikes / don't care / "am afraid of this". And finally I am not convinced that this actually means something (it is not uncommon for people to "like" something in the abstract but then not "like" it in practice once it is there)
But most importantly to me, the set of people who even see these discussions, and comments/likes, is highly self-selected: People who see it are likely to already be interested in enforcing standards, of dealing with the resulting work, they might even read the release notes for Aqua and opt-in to new checks. (Plus: one can only "like" in Discourse, there is no way to "dislikes"; and 7 or 15 are also not very impressive numbers to start with)
But in my work and daily interactions with people who need to work with Julia code and packages but just want to get stuff done, I see a very different picture. There Julia and package updates are seen as something to dread, as something that causes constant churn by introducing breaking changes. I am glad when I can convince these people to use Aqua at all. Each new test is a new hurdle that needs to be carefully weighed: how important is this test, how easy is it to understand what is even about (and why it matters) how hard is it to make it pass (other than turning it off)?
In my subjective view, adding ExplicitImports here is a step to far. No offense intended: I think it's wonderful this package exists, and I am sure it is beneficial for many people. I also think we should mention it (and some other tools and "best practices", such as JET) in the README and manual.
And JET, SnoopCompile etc. are all nice tools in theory, but like the "problems" this PR "revealed" in Aqua itself, they often are very tricky to resolve and require convincing other package authors and even Julia itself to make changes.
Some people love spending their time with that.
Others need to get their job done first.
Again, this is a tricky balance, and admittedly where the cutoff is is subjective. For me one rule of thumb is that stuff were a fix often requires changes outside of your own package is too far.
There is also the classic dependency problem: by tying Aqua to ExplicitImports we also become beholden to decisions you make: if ExplicitImports decides to make a minor release with a stricter check, then every Aqua user is exposed to that, and Aqua will receive the issues complaining about that.
If you really want a "one stop solution"), I suggest to make a new HardcoreAqua package (well, with a better name), which uses Aqua and ExplicitImports, but and if you like alsos enforce isempty(JET.get_reports(JET.report_package(YourPackage))) and run some SnoopCompile checks; and runs Runic to verify code formatting, and whatever you like.
There was a problem hiding this comment.
I think Aqua has a pretty general scope:
Auto QUality Assurance for Julia packages
There's nothing that actually unifies the particular set of checks it does under one theme, they are just a bunch of useful checks. So to me it seems weird to say whatever checks already exist are OK, but other checks are not. And test_all is inherently a one-stop-shop thing, it's run all the checks you can.
Here's a random idea:
- deprecate
test_all - introduce
test_all_v1which maps to the existing set of checks and will never get new checks nor get deprecated - introduce
test_all_v2which adds ExplicitImports' checks - future check additions do not need to be a breaking release of Aqua, they are instead a new function
So anyone can easily stick with test_all_v1 forever, but also can keep up with compat and bugfixes (don't need to be stuck on an old version of Aqua). Then if they want to opt-in to more checks, they can go to v2.
Could also have Aqua.latest_check_version() = 2 so users can get test failures if they aren't on the latest version, if they want, or introduce test_all_latest.
There was a problem hiding this comment.
Instead of "versions" we could call these "levels of strictness". Like e.g. test_all(;strictnesslevel=1). Or make a type out of it: test_all(StrictnessLevel(1)) which then allows to have test_all(StrictnessLevelHighest()). Or what ever else makes s good API.
It was requested in #268 by a few people; I think the idea is that an all-in-one solution is preferred for package maintenance checks. I added ExplicitImports checks 7 kinds of problems (copying the table from the readme):
I think these are all good best-practices and make sense to be on-by-default in Aqua besides implicit imports. To me that one should be opt-in. But I do think the integration adds value to catch the other issues. |
|
Thanks for reviewing @fingolfin. Please ignore the formatting of non-affected parts in the file, it was my autoformatter and I'll undo the changes.
I can't fix all of the warnings for Aqua itself because the package relies on a lot of private symbols, e.g. from Base and Core. If you prefer I can make some more modifications to the source code and remove a few of the names in the "ignore" lists, but they won't be empty either way. I wanted to make the PR easier to review, which is why I went with minimal modifications of the source.
I agree with @ericphanson that the idea is to centralize good practices, so that people don't have to manually combine several packages that are all related to code quality. But if we centralize these checks and leave them deactivated by default, no one is gonna bother to read the Aqua release notes and turn on new checks in their test suite. So, to some extent, the idea is to force good practices on people, with the hope that 1) it is generally good for beginners to stick to them (with the possible exception of the first check on implicit imports) and 2) experienced users will know which ones to turn off if they don't want them. As shown in #268 (comment) (7 likes) and https://discourse.julialang.org/t/i-find-it-hard-to-develop-in-julia/135497/58?u=gdalle (15 likes), the idea has gathered quite a lot of enthusiasm in the community. I would kindly ask you to reconsider your stance on its value :) |
|
I would definitely prefer that Aqua just did it all... I don't want to do 5 different packages for this. If it's not Aqua, then what package does it? I'll use whatever one does. |
|
In my opinion, any additional check that significantly increases the quality of my package is worth it. Since there is no argument that ExplicitImports adds value, then I think it's worth it. The headache, extra work, verbosity, breakage, etc are all worth it in the sense that if you want to significantly increase the quality of your package you'll just have to put in the work. Including it in Aqua just makes Aqua all the more useful and meaningful. If you want something quick that's easy and means less in terms of quality then don't add Aqua to your tests. I guess it gets tricky if there are cases where ExplicitImports incorrectly fails a quality check (i.e. packages that are totally fine despite what ExplicitImports says AND there is no way to satisfy ExplicitImports for those packages). But Aqua has mechanism to game that ("test less"). |
|
I have always seen Aqua as something that all packages should be tested with. Assuming no false positives, you shouldn't need to disable any of the tests. No one should have stale dependencies or type piracy in their package. I'm not sure I see enough of a benefit in explicit imports to see that strong of a recommendation. I didn't realise that Aqua even had tests that weren't turned off by default. Perhaps having tests that aren't enabled by default would be okay if there was more of a focus on it in the documentation (you currently need to ignore the fact that there is a function called That way, users who want a one-stop-shop solution and deeply care about their code quality (the same users who would spend the time reading the Aqua docs, and enabling the extra tests they want) can have access to that. While users who would be more likely to ditch Aqua if it told them they need to rewrite all their |
|
I am very much in favor of this. I already use the ExplicitImports tests in several packages I am maintaining. IMHO having no implicit imports very much can help to read package sources for those who want to understand them, or even want to contribute.
This reminds me of this discourse thread... My feeling is that testing non-public import/access will lead to quite some needs for fixes in packages by avoiding these imports, by marking symbols meant to be public as public, or by package developers asking to add internal functionality to the API (both for Julia and for packages). Also in that thread there is a nice description of a way to proceed in these cases. See also JuliaPluto/Pluto.jl#3472. I think the announcement of Aqua 1.0 should address this point upfront and provide corresponding recommendations (may be in the docs) how to proceed. |
I find this kind of statement puzzling: you are already using it. So how would you benefit from this PR then? |
|
I find it sad that my points are mostly ignored... not refuted, but ignored:
|
|
Maybe a relatively quick solution that will address Max's concerns would be to have a fairly centralized flag in Aqua that turns it "opinionated and harsh" and have it set to false by default but have it relatively loud in logs. E.g. if it is not set do |
Well, for my packages - like @ChrisRackauckas said - to have just a single one-stop dependency for package quality tests. In the moment I test for Aqua, for undocumented names, and for explicit imports. Also I believe that this will help to improve the general quality of the package ecosystem. In particular the highlighting of uses of non-public functionality. As I tried to explain, this might be a quite non-trivial transition. EDIT: @fingolfin: I seem to get your point - to organize this, indeed we might need to split this e.g. into Aqua and SuperAqua. EDIT: or have different levels of strictness. |
|
If you want my two cents: Easy implicit imports were a huge mistake. I'd love if in Julia 2.0, It makes it impossibly hard to reason about any Julia code. Even for new package READMEs, usage example are often hard to understand, because I can't actually tell that a particular function is from the package if I don't have the public functions from Julia and any other package used in the example memorized. So anything we can do to eradicate implicit imports from the ecosystem is a good thing. Please do make this a default. |
why a separate package over my proposal to introduce versioned entrypoints? Bc of the dependencies? |
|
There are two entangled issues here: 1. What belongs in the default, unopinionated package tests? Due to the name of the package and the name of the PR, I think the true purpose of ExplicitImports was a bit forgotten in the discussion above. The "explicit imports" check is actually just one of seven (see this comment), and probably the most opinionated one, which is why I agree with @ericphanson that it should be deactivated by default as far as 2. Where should more opinionated, but still useful tests go? In this category, I'd put the "explicit imports" check mentioned above, and possibly other things like
In any case, the burden on users has been a bit overstated: just turn any individual check in Aqua off if you don't want it. Taking the case of the current PR, you don't have to go through the trouble of listing private symbols to ignore if you're a beginner, you just need to set |
|
Gentle bump on this one: based on the discussion and feedback here, is it worth pursuing or should I just give up? |
|
I really don't understand the gatekeeping here. The least common denominator seems to be add the test but make it opt-in. Can we just do that, please, and move the other discussion to a separate issue? What frustrates me the most in the whole Julia landscape is that there are so many really good PRs that stall for (seemingly) minor reasons. If there are architectural issues, then okay, those need to be addressed. This doesn't seem to be the case here, however. If one does not like this feature/check/... then don't use it. Please don't prevent the people who do see value in it from using it (or improving quality of life to use it). |
|
I only skimmed the discussion and missed the open question on whether to enable new tests by default in the previous comment. I would be fine with either option (as to not delay this PR further). My tendency is towards making new tests opt-in, i.e., to disable them by default. Updating any package dependency to a breaking version usually requires care, meaning that a developer keeps an eye on breaking tests anyways, especially since Aqua v1.0.0 is on the horizon. This would also have the benefit of diversifying the Aqua badges: this package fulfills Aqua version X tests. Regarding the opt-in, this could be as easy as |
Look, that's strong words to throw around. Can you perhaps at least consider that someone might be honestly concerned about things like added complexity, extra dependencies, feature bloat etc.? You are presenting it as if this PR decided over whether one can use ExplicitImports or not. In reality, anyone can use it by adding two lines to their So yeah, this "saves" something for users of Aqua, but this a far cry from
At least you qualify that statement by adding
OK, that's something I can see, for some (perhaps a lot?) people, not having to add a few lines and instead only having to add one line is a perhaps improving their quality of life. More importantly, the more convenient it is to use a feature, the more likely it is used, I fully get that. What I disagree on is whether this is a useful tradeoff. I just don't find ExplicitImports as much of a "no-brainer" as others here, see e.g. JuliaLang/julia#61208 To be clear: I think it is useful and am glad it exists, but my own experiences when trying to use it in a large package were mixed (false positives in macro definitions was another pain point; I should write up an issue for that). But that's not my main reason for hesitating: rather, right now Aqua has virtually no dependencies, and just does 'basic' stuff. People are already are clamoring for also adding in JET, SnoopCompile, even code formatters. That's an understandable goal but changes the scope of Aqua quite a lot. It also changes the maintenance effort quite a bit: now one has to watch our for updates and changes in the packages and keep with them. This is highly non-trivial for at least JET and SnoopCompile in my experience. Hence my suggestion that those people who want such a mega-package, the "ultimate QA package", then those people should create such a package, which can use Aqua and other tools, and they can then take care of maintaining it. Easy-peasy, no?
Oh, I totally get that (but I don't think at all that this is specific to Julia, it is a typical thing in many open source projects). And it has its bad sides... but it sometimes also has good reasons. E.g. I am experiencing it right now myself in a Julia PR where I am trying to add a feature whose absence has baffled me forever. The functionality and code is not even in question, the argument is about what to call the function, and that ends up very tiring. Easy to get out the "bikeshedding" card. But the flip-side is this: the naming is so difficult because the naming of the existing functions is a mess... If only a few PRs years ago had not been merged so quick and had undergone some more thinking and revisons... As someone who has been maintainer of many different small and larger open source projects for the past 25+ years, this is a constant struggle: do you accept contributions quickly to encourage contributions; or do you insist on passing some barriers because while that that contributor may move on next week, you expect to be around for a couple years more in the project, and will have to deal with any fallout... This perspective changes how you view things. That said: I did not create this package originally, and now @lgoettgens is most active maintainer (though in my private discussions with him, his views seemed similar to mine, he just didn't want to get dragged into this tiresome discussion and was happy to let me comment). Anyway, another option is that we (well, me, I can't and won't speak on this for @lgoettgens) step down as maintainer and those who want to change directions put their money where their mouth is and deal with the issues and fallout and maintenance. (To be clear, that's not meant as a threat, more an offer: if I am blocking progress, then I'll step aside, no problem) |
|
I'm currently porting some of my code to Python, a language I'm less fluent in than Julia. In this process, I've greatly benefitted from A simple CLI call to One of Julia's strengths over the Python ecosystem is that we are much less fragmented in our tooling. I've always seen Aqua.jl as the ruff of Julia and hope that we won't need to fragment the ecosystem into different Aqua forks to have a unified linting interface. |
Fixes #268:
Aqua.test_explicit_importswhich forwards everything toExplicitImports.test_explicit_imports(see its documentation)Main questions:
Ping @ericphanson