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

Clarify extension docs about defining new symbols #3552

Merged
merged 3 commits into from Nov 15, 2023
Merged

Clarify extension docs about defining new symbols #3552

merged 3 commits into from Nov 15, 2023

Conversation

gdalle
Copy link
Contributor

@gdalle gdalle commented Jul 21, 2023

Partial fix for #3499

It's a very minimal addition and I would welcome more contributions

Partial fix for #3499 

It's a very minimal addition and I would welcome more contributions
Copy link

@ffevotte ffevotte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the nice write-up! Seeing the frequency with which these questions come up on discourse, I think it's important to add this kind of details to the documentation.

docs/src/creating-packages.md Outdated Show resolved Hide resolved
```julia
const MyFancyContourPlot = Base.get_extension(Plotting, :PlottingContourExt).MyFancyContourPlot
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also mention here the "recipe" where the parent package provides a way to access things within its extension?

Like

# within the Plotting module:
function get_ext()
    ext = Base.get_extension(@__MODULE__, :PlottingContourExt)
    if ext === nothing
        error("extension not loaded...")
    else
        return ext  # or ext.something_defined_in_ext
    end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of this! Is it widely used?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that here: https://discourse.julialang.org/t/are-extension-packages-importable/92527/15

Not sure whether it's widely used, but it seemed useful...

@timholy
Copy link
Sponsor Member

timholy commented Jul 21, 2023

It's worth noting there's another pattern that was initially described in #3282 but which I then stripped out:

module MainPackage

export stub
function stub end   # no methods

# probably a good idea to `register_error_hint` in `__init__` describing what other package(s) must be loaded to make `stub` active

end

########################

module MainPackagePlotting  # extension package

MainPackage.stub(obj::SomeObject) = ...

end

@KristofferC
Copy link
Sponsor Member

I personally don't think that this clarifies very much. And what is considered good practice is a bit hard to say at this point since the feature is so new. The first step should imo be to describe how everything works and do that well. Once that is done, there can be a discussion of best practices.

So the first question I would have is, are there currently lack of documentation how things work on a technical level (like how the name spacing of symbols defined in the extension work etc)?

@ffevotte
Copy link

ffevotte commented Jul 21, 2023

are there currently lack of documentation how things work on a technical level (like how the name spacing of symbols defined in the extension work etc)?

One thing that I think is definitely lacking is a documentation for get_extension (which I think is currently only mentioned as the way to check whether the Julia version supports extensions via !isdefined(Base, :get_extension).)

Apart from that, I think all information is already there for users to understand how things work on a technical level. But judging by the questions asked on discourse, what appears to be missing is some form of "How to (do what I want)?"

AFAIU, this PR tries to provide answers to questions asked on discourse that all look like: "I used to use Requires.jl to define new functions/types/variables in my package when some optional dependency is also loaded. When switching to weak dependencies, these functions/types/variables are all namespaced into the extension module: how do I access them?"
Looking at the current documentation, it seems (to me at least) relatively clear why users are having such issues: because the extension is a new module, that comes with its own namespace. What isn't clear (IMO), is: what to do about it? I think this is what this PR tries to address.

Another way to look at this is the discussion in Backwards compatibility. This section gives a nice How-To guide about how to transition from (Requires.jl or strong dependencies) to weak dependencies. However, the "recipes" given there are only valid under some assumptions, (e.g. that the conditionally loaded code only adds new methods to functions already defined elsewhere). So the techniques discussed in this paragraph should be complemented with ways to solve issues appearing when these assumptions are not valid (or warn the user to not use extensions/weak deps in such cases).


Writing all of this made me think of an alternate way of incorporating this in the documentation:

  • in the main paragraph, perhaps simply note more explictly that since the extension has its own module, everything defined in it is namespaced (and therefore "insulated" from the main package namespace)
  • in the discussion about backward compatibility & transition from Requires.jl, add some advice about how to make things defined in the extension available from the outside:
    • (a) add methods to functions already defined (and accessible from) elsewhere
    • (b) possibly define empty function stubs to make method (a) relevant
    • (c) use get_extension in some way to get direct access to the extension module (either from the main package or the client code)

@gdalle
Copy link
Contributor Author

gdalle commented Jul 24, 2023

It's worth noting there's another pattern that was initially described in #3282 but which I then stripped out:

That's what I meant when I mentioned "zero-method functions", perhaps I should be more explicit?

And what is considered good practice is a bit hard to say at this point since the feature is so new. The first step should imo be to describe how everything works and do that well. Once that is done, there can be a discussion of best practices.

I based my PR mostly on comments by you that I had read on Slack or Discourse, like this one:

Well yes, but I do think that best practices should be to make the functionality exposed by overloading functions in the parent. It can even be a zero method function that is overloaded. So you would not need to access the namespace of the extension.

But that was shortly after the 1.9 release. Now that we have a little more hindsight, would you say that accessing the extension namespace directly is a good idea?

Are there currently lack of documentation how things work on a technical level (like how the name spacing of symbols defined in the extension work etc)?

I think the use of get_extension to actually access the namespace, and not just check whether it exists, is an important addition

@gdalle
Copy link
Contributor Author

gdalle commented Jul 24, 2023

Writing all of this made me think of an alternate way of incorporating this in the documentation:

I understand where this comes from but I'm not sure it's a good idea to wait that long in the structure of the docs. Basically I'm afraid that the "backwards compatibility" section sounds too technical, and that people might just skip it.

Besides, there are many developers who never touched extensions before since they were not an official part of the language, but who will try them out now. Their packages may never have used Requires, and they may not even care about compatibility < 1.9. Yet these developers also need the info.

@gdalle
Copy link
Contributor Author

gdalle commented Aug 2, 2023

So where do we want to go with this? Should I just remove the notion of good practice? I personally don't think so, even in the extensions talk at JuliaCon it was rather clear that extending functions was the safe way to go

@gdalle
Copy link
Contributor Author

gdalle commented Aug 28, 2023

Gentlest bump on this, I'm more than willing to amend the PR if we agree on the direction

@KristofferC
Copy link
Sponsor Member

To me, the core information to convey is that you cannot create new symbols in the parent package from the extension. Once that is established, everything else follows from that.

It is however totally fine to define new structs and functions in an extension and access them from the parent package with Base.get_extension.

@gdalle
Copy link
Contributor Author

gdalle commented Aug 28, 2023

Sounds good and clear, I'll rephrase in that direction

@gdalle gdalle changed the title Defining new objects in extensions is possible but bad practice Clarify extension docs wrt new symbols Aug 28, 2023
@gdalle gdalle changed the title Clarify extension docs wrt new symbols Clarify extension docs about defining new symbols Aug 28, 2023
@gdalle gdalle requested a review from ffevotte August 28, 2023 14:34
Copy link

@ffevotte ffevotte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me; many thanks @gdalle!

I'm sure we'll have much more to say on this topic in a few months/years when more feedback on extensions will be available. But in the meantime, it looks to me like this proposal gives the two most useful (IMO) pieces of information:

  1. extending (possibly empty) functions is the most common use case for extensions
  2. Base.get_extension can be used to retrieve the extension module from its "outside".

docs/src/creating-packages.md Outdated Show resolved Hide resolved
@gdalle
Copy link
Contributor Author

gdalle commented Aug 29, 2023

@KristofferC this should be better now

@StefanKarpinski
Copy link
Sponsor Member

It would be nice to get some kind of guidance along these lines in the docs. @KristofferC, are you happier with this now? Or could you suggest something that would make more sense to you?

@jordancluts
Copy link

I am not remotely informed enough to express an opinion on the details discussed in this thread. I did, however, want to emphasize that something should be done to address this in the docs. There are near daily questions on Slack about confusion when defining functions/classes in an extension. It is obvious that the docs need to guide new users better (whether that is telling them not to do it, or how to go about doing so). I'm hoping that this anecdote will spur something getting over the finish line to help all the folks that are running headlong into this problem.

@gdalle
Copy link
Contributor Author

gdalle commented Sep 12, 2023

Whatever is needed to get this PR merged, I will gladly do ^^

@jacobusmmsmit
Copy link

I think it's better to have something in the docs that might change in the future or is possibly imperfect or incomplete rather than nothing. The ideas described by the PR might not be the best, nor the way that extensions will be used in the future, but they do provide a consistent way to work with extensions and give an idea of best practices.

@gdalle
Copy link
Contributor Author

gdalle commented Sep 26, 2023

@KristofferC gentle ping

@gdalle
Copy link
Contributor Author

gdalle commented Oct 5, 2023

Is there anything that needs changing here?

@gdalle
Copy link
Contributor Author

gdalle commented Nov 5, 2023

Bump in case anyone is watching, this is a 15-second review

@KristofferC KristofferC merged commit fd00e3f into JuliaLang:master Nov 15, 2023
13 checks passed
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.

None yet

7 participants