-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
recommend explicit using Foo: Foo, ...
in package code (was: "using considered harmful")
#42080
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also say that you can just do import Foo
instead of using Foo: Foo
?
What's the status of tooling support here? I'd say that's required before we make any such proclamation. |
Good point about tooling. Something very nice that Racket's IDE DrRacket does is that it shows you the source of a binding when you hover over it. That very nicely mitigates any confusion when reading code over the source of a particular binding. Also note that my suggested change to the verbiage makes this more informative and less prescriptive, which could be worthwhile regardless. |
Personally, I think we should be more prescriptive. As Kristoffer points out, this kind of use of |
I think it should be left up to code authors how they want to write their code. We can provide information that may inform their decision, but saying "Julia provides a way for you to do this but don't do it lul" isn't helpful. People find and fix breakages on the (in my experience, rare) occasions that they occur, users can qualify bindings or do explicit loads if they choose. When Base adds a new export, we should probably run PkgEval to see what effect it has. It would also be nice if packages with many dependants had reverse testing on new tags. That way package authors can be better equipped to work together to provide a good experience for users. |
People (including me) don't read documentation carefully always. Starting with a concise recommendation followed by reasoning seems to be the best practical strategy. If they disagree with the recommendation and understand the reasoning, it is of course "left up to code authors how they want to write their code." One missing reasoning step in the current version is that, adding features 1 is not considered breaking and can be done without incrementing the major version according to semver. So, if you only bound the major version and not minor version, you simply cannot 2 use Footnotes |
In many situations, I favor In any case, I haven't seen a good defense for The admonition seems like it belongs in some kind of style guide. And tooling might include an option in linters, with an option at some level to error on warnings. I don't see why it needs a certain level of tooling support before the recommendation is published. But, without tooling, I suspect adoption will remain weak. I'm thinking of the ways PEPs and pylint work in Python projects and clippy in rust. I not suggesting the big administrative burden of the whole PEP system, just that starting to adopt styles and tooling to handle complexity as the amount of code increases is wise. |
It could be an option for |
Arguably, there's nothing new in this PR. It has been (more or less) possible to derive this from the semver specification. While discussion on tooling is important on its own, I believe a more fundamental aspect is the interaction of semver and Julia's package/module system. Making it crystal clear even for those who haven't read the semver specification will help to reduce "controversies" of choosing the behavior of such tools. (I'm sorry for RTFM'ing. But I feel that this is an important point here.) |
I would instead recommend the other side: be conservative on what you should export and think about the naming ambiguity. For instance, exporting a From a maintenance perspective, exporting a function at will also makes it very difficult to deprecate and remove the symbol. |
I think this is perhaps stronger advice than required. How about pointing out the problem and proposing a way to address it without prescribing that "you shouldn't do this." Personally I'm OK with a rare breakage if I get to write nicer code internally in my packages. |
Out of curiosity, why? The presence or absence of tooling doesn't change any of the facts. |
Sure, this is why this is only a recommendation and this section just states the recommendation and the reason for it (exporting new names becomes a breaking change). |
But the language is still quite prescriptive: "X is not recommended, do Y instead" vs "X can be messed up by Z, you can prevent this by doing Y". Subtle difference, but one that I think affects perception. |
Just to give you an example of what this leads to (JuliaWeb/HTTP.jl#745 (review)). HTTP.jl now does type piracy on And with "nicer" code you mean to avoid the annoyance of having to spell out where your names come from. While you might be smart enough to remember exactly where all the names come from, I am not, so trying to look up where certain names come from is one of the main annoyances I have while reading code. Explicit mentioning where names come from would be a nice help to future contributors who don't have the namespace of all the dependencies in their head. |
Yes, the "axiom" here is that assuming everyone following semver and declares compatibility correctly, then we want to encourage coding in a way that keeps the code after a package update. That's why we discourage the use of type piracy, we discourage using internals of packages/Base. And as you say, an assumption is that adding a new export is not considered a breaking change (which I think is quite uncontroversial, looking at how Base does it). The conclusion from that is that since |
Any particular reason for not backporting this? I imagine that people using the LTS will be reading the 1.6 docs, and it would be good for them to see this recommendation. |
I agree that having tooling is not a pre-requisite for merging this PR. |
I'd be okay with a descriptive warning without there being tooling support. I don't like having a prescriptive admonition without tooling support. With tooling support, I'm 100% for it (hence my conflicting thumbs). Perhaps I'm in the minority, but I don't think this is a minor point. To make a (poor) analogy, some new governmental regulations are accompanied by tax credits/grants/assistance to offset their implementation cost. We should make it easy for folks to address. Without that, it's just words that core devs will point at to blame packages for breaking. |
Similarly, I think a descriptive warning (like Alex's suggestion) could be back ported to 1.6/1.7, but a prescriptive one should not be. |
The point is that the "regulation" (= semver) already exists. (I don't believe tooling should be a prerequisite but I'd also point out creating one is straightforward thanks to JuliaFormatter.jl. Here's my POC I experimented a while ago for automatically generating |
This is such a silly and tired argument at this point. Can we just move forwards with this PR and/or #53428 ? |
Note: the base branch (target branch) of this PR is `kc/warn_using` (#42080), NOT `master`. --- This is additive to #42080 which adds advice to the docstring of `using` . The wording here is a tweaked version of @IanButterworth's suggestion here: #42080 (comment) --------- Co-authored-by: Max Horn <[email protected]> Co-authored-by: Dilum Aluthge <[email protected]>
I've merged #53428 into this PR. |
Well I do not think that we will gather 100% unanimous support here. However, it's worth noting that all the concrete objections here have been addressed since this was originally proposed three years ago:
I think the recommendation (and associated information it contains) is highly valuable, even if you don't agree that this is problematic or worth incorporating into your own personal code style. I fully support a merge at this point. |
using Foo
in package code (or "using considered harmful")using Foo: Foo, ...
in package code (was: "using considered harmful")
Perhaps add a reference to this package? I have found it stable enough and even enabled it in CI for some packages to catch mistakes. |
My approving review indicates that the content itself is well written and mergable. I'm still ambivalent about the sentiment. We should discuss this at the next triage call in about 42 hours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely better than the initial version of the PR. I made some additional suggestions in-line, but the part I think is very important to note that's currently unstated is that it's not an error if two or more modules export the same binding. Doing so is a common pattern across the ecosystem, so this is potentially confusing as written for new users.
Triage thinks that a good pragmatic solution here is to accept @ararslan's suggestions and merge. |
Co-authored-by: Alex Arslan <[email protected]>
Co-authored-by: Alex Arslan <[email protected]>
Qualifying the names being used as in `using Foo: Foo, f` is | ||
recommended over plain `using Foo` for released packages, and other | ||
code which is meant to be re-used in the future with updated dependencies | ||
or future versions of julia. | ||
|
||
The reason for this is if another dependency starts to export one of the | ||
same names as `Foo` and you attempt to use that name, then previously working | ||
code will error due to an ambiguity in which package the name should be | ||
taken from. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synchronizing the wording between the docstring and the manual
Qualifying the names being used as in `using Foo: Foo, f` is | |
recommended over plain `using Foo` for released packages, and other | |
code which is meant to be re-used in the future with updated dependencies | |
or future versions of julia. | |
The reason for this is if another dependency starts to export one of the | |
same names as `Foo` and you attempt to use that name, then previously working | |
code will error due to an ambiguity in which package the name should be | |
taken from. | |
When two or more packages/modules export a name and that name does not refer to the | |
same thing in each of the packages, and the packages are loaded via `using` without | |
an explicit list of names, it is an error to reference that name without qualification. | |
It is thus recommended that code intended to be forward-compatible with future versions | |
of its dependencies and of Julia, e.g. code in released packages, list the names it | |
uses from each loaded package, e.g. `using Foo: Foo, f` rather than `using Foo`. |
Looks like someone just needs to commit @ararslan 's suggestion and then merge |
I feel we are heading up against a "
using
crisis" where any new feature that is implemented by exporting a new name (either in Base or a package) becomes a breaking change. This is already happening (JuliaGPU/CUDA.jl#1097, JuliaWeb/HTTP.jl#745) and as projects get bigger and more names are exported, the likelihood of this rapidly increases.The flaw in
using Foo
is fundamental in that you cannot lexically see where a name comes from so when two packages export the same name, you are screwed. Any code that relies onusing Foo
and then using an exported name fromFoo
is vulnerable to another dependency exporting the same name.Therefore, I think we should start to strongly discourage the use of
using Foo
and only recommendusing Foo
for ephemeral work (e.g. REPL work).