Add guidance for using qualified names in packages #53428

MasonProtter · 2024-02-22T10:38:40Z

Note: this PR was merged into #42080. It was NOT merged into master.

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)

DilumAluthge · 2024-02-22T17:17:57Z

Can/should we merge this into #42080?

MasonProtter · 2024-02-22T19:16:05Z

If we think that would help get either of them merged faster then sure. @KristofferC, do you want to merge this branch into yours? Or should we just keep them as separate PRs?

fingolfin · 2024-02-23T08:27:05Z

doc/src/manual/modules.md

-brings *only* the module name into scope. Users would need to use `NiceStuff.DOG`, `NiceStuff.Dog`, and `NiceStuff.nice` to access its contents. Usually, `import ModuleName` is used in contexts when the user wants to keep the namespace clean.
-As we will see in the next section `import .NiceStuff` is equivalent to `using .NiceStuff: NiceStuff`.
+brings *only* the module name into scope. Users would need to use `NiceStuff.DOG`, `NiceStuff.Dog`, and `NiceStuff.nice` to access its contents.
+As we will see in the next section `import .NiceStuff` is equivalent to `using .NiceStuff: NiceStuff`. Usually, `import ModuleName` or `using ModuleName: ModuleName` is used in contexts when the user wants to keep the namespace clean.


Why the sudden switch from NiceStuff to ModuleName ? I am fine with either name, but shouldn't it be used consistently, i.e. either everything with NiceStuff or else everything with ModuleName?

That wasn't really my choice, I was just modifying the existing docs with its existing name scheme. See: https://docs.julialang.org/en/v1/manual/modules/#Standalone-using-and-import

FWIW I think maybe the that could be overhauled, but I also don't think it's necessary.

fingolfin · 2024-02-23T08:27:29Z

doc/src/manual/modules.md

@@ -168,6 +168,23 @@ Importantly, the module name `NiceStuff` will *not* be in the namespace. If you
 julia> using .NiceStuff: nice, DOG, NiceStuff
 ```

+!!! note
+    Qualifying the names being used as in `using NiceStuff: NiceStuff, nice` is recommended over plain
+    `using Foo` for released packages, and other code which is meant to be re-used in the future with


A third name, Foo... Let's stick with one, e.g. NiceStuff perhaps?

Suggested change

`using Foo` for released packages, and other code which is meant to be re-used in the future with

`using NiceStuff` for released packages, and other code which is meant to be re-used in the future with

Right, yeah this naming conflict should be avoided. I think for the purposes of this !!! note though, maybe it'd be better to use a new set of names Foo and Bar because we want to talk about conflicts, and we also want this !!! note section to be readable standalone.

fingolfin · 2024-02-23T08:27:54Z

doc/src/manual/modules.md

+    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` the code will error due to an ambiguity in which


Suggested change

same names as `Foo` the code will error due to an ambiguity in which

same names as `NiceStuff` the code will error due to an ambiguity in which

doc/src/manual/modules.md

fingolfin · 2024-02-23T08:32:25Z

doc/src/manual/modules.md

+    with the future releases of Foo 1.x and Bar 2.y which both export a
+    function `baz`).


If I didn't already know what this is about, I am not sure I would understand. I think the point you are trying to make is: suppose you are doing using Foo, Bar, and it works. But then Foo 1.1 exports a new symbol baz, while Bar already exports a different baz. Then suddenly using Foo, Bar fails.

Co-authored-by: Max Horn <max@quendi.de>

MasonProtter · 2024-02-23T10:39:05Z

@fingolfin I've changed the !!! note section to only talk about Foo and Bar. You might be right that it's better to just stick with NiceStuff here but I think that's not ideal because it'd be good if this note was understandable on its own, but the NiceStuff example has some baggage associated with it like being defined inside Main and so we'd need to write using .NiceStuff instead of using NiceStuff.

I don't have a super strong opinion here, so let me know what you think of this change.

doc/src/manual/modules.md

ararslan · 2024-02-28T09:56:52Z

doc/src/manual/modules.md

-brings *only* the module name into scope. Users would need to use `NiceStuff.DOG`, `NiceStuff.Dog`, and `NiceStuff.nice` to access its contents. Usually, `import ModuleName` is used in contexts when the user wants to keep the namespace clean.
-As we will see in the next section `import .NiceStuff` is equivalent to `using .NiceStuff: NiceStuff`.
+brings *only* the module name into scope. Users would need to use `NiceStuff.DOG`, `NiceStuff.Dog`, and `NiceStuff.nice` to access its contents.
+As we will see in the next section `import .NiceStuff` is equivalent to `using .NiceStuff: NiceStuff`. Usually, `import ModuleName` or `using ModuleName: ModuleName` is used in contexts when the user wants to keep the namespace clean.


when the user wants to keep the namespace clean.

There's nothing that says what it means for a namespace to be "clean," nor how changing how you load a package affects cleanliness. Since the exported names of a module aren't resolved until they're referenced, using doesn't "pollute" the enclosing namespace in the same way that constructs from other languages do.

This isn't really material to this PR. I was just moving around existing text and adding that there's the option to do using ModuleName: ModuleName as well.

I'm not suggesting it be added, I'm challenging the wording used, as the motivation as written isn't particularly useful.

Again though, that's not something new in this PR.

I agree the "clean" bit is not nice -- but it was already there before. It seems unfair to hold this PR hostage over the author not "improving even more things".

Perhaps I am misunderstanding what you'd wish to have changed, @ararslan, in that case I'd appreciate if you could clarify what changes you are requesting exactly.

@ararslan If this is the only part of the diff that you have concerns about, then we can revert it from the PR and make this change in a separate PR, just in the interest of getting this PR in?

cc: @MasonProtter

So, in the interest of moving this PR forward, I have removed this specific small change from this PR in 862a546 and moved it to a new PR (#54575).

… is used

doc/src/manual/modules.md

…hat there's the option to do `using ModuleName: ModuleName` as well" I'll create a separate PR that contains only this small change.

The review comments have been addressed

DilumAluthge · 2024-05-24T23:54:19Z

I've been giving some more thought as to whether this PR should be merged into #42080 (as opposed to merging this PR directly into master).

My guess is that if someone has concerns or objections to #42080, then they'll probably have those same concerns/objections to this PR. So it probably makes sense to merge this PR into #42080, so that the main discussion can occur in #42080 (which is where the majority of the discussion has been happening thus far).

Also, I don't want to give the impression that merging this PR into master would be doing a kind of "end run" around the PR review happening in #42080.

Therefore, I propose that we change the target branch (base branch) of this PR (from master to kc/warn_using) and then merge this PR into #42080.

I'll wait a little bit, and then if there are no objections to that plan, I'll perform those steps.

cc: @KristofferC @mbauman

DilumAluthge · 2024-06-02T23:06:55Z

Alright, hearing no objections, I'll merge this PR into #42080 (instead of into master).

DilumAluthge · 2024-06-03T21:06:20Z

I've merged this PR into #42080.

Add guidance for using qualified names in packages

ce554cf

MasonProtter mentioned this pull request Feb 22, 2024

recommend explicit using Foo: Foo, ... in package code (was: "using considered harmful") #42080

Open

typo

0a8182e

MasonProtter added the domain:docs This change adds or pertains to documentation label Feb 22, 2024

MasonProtter added 2 commits February 22, 2024 11:43

Update modules.md

7e0b4d7

fix whitespace

e464600

DilumAluthge requested review from IanButterworth and KristofferC February 22, 2024 17:18

LilithHafner mentioned this pull request Feb 22, 2024

Make Base exports yield to package exports #53442

Closed

fingolfin reviewed Feb 23, 2024

View reviewed changes

MasonProtter force-pushed the patch-16 branch 2 times, most recently from 4983382 to e464600 Compare February 23, 2024 10:00

MasonProtter and others added 5 commits February 23, 2024 11:02

Update doc/src/manual/modules.md

f2023a2

Co-authored-by: Max Horn <max@quendi.de>

re-write info example

455c7bf

whitespace

a3a9d63

typos

921aaa4

more whitespace

f792ff4

ararslan previously requested changes Feb 28, 2024

View reviewed changes

clarify that the ambiguity error is only raised if the ambigious name…

7263ac3

… is used

mbauman approved these changes Apr 19, 2024

View reviewed changes

Merge branch 'master' into patch-16

810c4e9

fingolfin approved these changes May 24, 2024

View reviewed changes

fingolfin and others added 2 commits May 25, 2024 00:39

Merge branch 'master' into patch-16

4a5ed51

Merge branch 'master' into patch-16

8996a87

DilumAluthge approved these changes May 24, 2024

View reviewed changes

DilumAluthge reviewed May 24, 2024

View reviewed changes

doc/src/manual/modules.md Outdated Show resolved Hide resolved

Revert a small change that "mov[es] around existing text and add[s] t…

862a546

…hat there's the option to do `using ModuleName: ModuleName` as well" I'll create a separate PR that contains only this small change.

DilumAluthge mentioned this pull request May 24, 2024

Small change to docs for module import syntax #54575

Open

DilumAluthge requested a review from ararslan May 24, 2024 23:22

DilumAluthge changed the base branch from master to kc/warn_using June 2, 2024 23:07

Merge branch 'kc/warn_using' into patch-16

948eece

DilumAluthge merged commit 5a497c8 into JuliaLang:kc/warn_using Jun 3, 2024
7 checks passed

MasonProtter deleted the patch-16 branch June 3, 2024 22:06

mbauman added a commit that referenced this pull request Jun 4, 2024

Unify some of the language from #53428

aa0ce07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add guidance for using qualified names in packages #53428

Add guidance for using qualified names in packages #53428

MasonProtter commented Feb 22, 2024 •

edited by DilumAluthge

Loading

DilumAluthge commented Feb 22, 2024

MasonProtter commented Feb 22, 2024

fingolfin Feb 23, 2024

MasonProtter Feb 23, 2024

fingolfin Feb 23, 2024

MasonProtter Feb 23, 2024

fingolfin Feb 23, 2024

fingolfin Feb 23, 2024

MasonProtter commented Feb 23, 2024

ararslan Feb 28, 2024

MasonProtter Feb 28, 2024

ararslan Feb 28, 2024

MasonProtter Feb 28, 2024

fingolfin May 24, 2024

DilumAluthge May 24, 2024

DilumAluthge May 24, 2024

DilumAluthge commented May 24, 2024

DilumAluthge commented Jun 2, 2024

DilumAluthge commented Jun 3, 2024

	`using Foo` for released packages, and other code which is meant to be re-used in the future with
	`using NiceStuff` for released packages, and other code which is meant to be re-used in the future with

	same names as `Foo` the code will error due to an ambiguity in which
	same names as `NiceStuff` the code will error due to an ambiguity in which

		with the future releases of Foo 1.x and Bar 2.y which both export a
		function `baz`).

Add guidance for using qualified names in packages #53428

Add guidance for using qualified names in packages #53428

Conversation

MasonProtter commented Feb 22, 2024 • edited by DilumAluthge Loading

DilumAluthge commented Feb 22, 2024

MasonProtter commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MasonProtter commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DilumAluthge commented May 24, 2024

DilumAluthge commented Jun 2, 2024

DilumAluthge commented Jun 3, 2024

MasonProtter commented Feb 22, 2024 •

edited by DilumAluthge

Loading