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

fix documentation of shared symbols #2263

Merged
merged 1 commit into from
Nov 7, 2021

Conversation

mahrud
Copy link
Member

@mahrud mahrud commented Oct 23, 2021

@DanGrayson
Copy link
Member

I'm not sure this pull request addresses the real issue, which seems to be that this documentation node refers requests for documentation of intersection to the page for intersect:

document {
    Key => { intersect, (intersect, List), (intersect, Sequence), intersection },
    Headline => "compute an intersection",
    PARA {
	"This function calculates the intersection of a list or sequence of compatible objects."
	},
    SeeAlso => {
	-- add references to intersect methods installed in packages _other than Core_ here
	"M0nbar::M0nbar",
	"NAGtypes::NAGtypes",
	"Polyhedra::Polyhedra"
	}
    }

@mahrud
Copy link
Member Author

mahrud commented Oct 29, 2021

No, I'm pretty sure it's addressing the problem. In particular, this isn't a problem only with intersection.

You can try separating intersection into it's own node in Macaulay2Doc and you'll get the same problem.

@mahrud
Copy link
Member Author

mahrud commented Oct 30, 2021

Try links to isEmpty and isEmpty(Polyhedron) in the documentation of Polyhedra. Here is the source:

<li><span><a href="../../Macaulay2Doc/html/_is__Empty.html">isEmpty</a> -- checks if a Polyhedron is empty</span>      </li>
<li><span><a title="checks if a Polyhedron is empty" href="_is__Empty.html">isEmpty(Polyhedron)</a> -- checks if a Polyhedron is empty</span>      </li>

The real issue, which this PR fixes, is that for instance this html file in the Polyhedra package is created incorrectly: https://faculty.math.illinois.edu/Macaulay2/doc/Macaulay2/share/doc/Macaulay2/Polyhedra/html/_is__Empty.html

In this case, the node in shared.m2 is simply

document { Key => isEmpty,      methodstr, SeeAlso => { "Polyhedra::Polyhedra",(isEmpty, RRi)} }

which isn't making any misdirections, as you suggest.

@DanGrayson
Copy link
Member

DanGrayson commented Nov 1, 2021

There should be no documentation node for "isEmpty" in the Polyhedra package, because "isEmpty" is in the core, and an error should be signaled when it tries to make one here:

document {
     Key => {isEmpty, (isEmpty,Polyhedron)},
     Headline => "checks if a Polyhedron is empty",
     Usage => " b = isEmpty P",
     Inputs => {
...

@mahrud
Copy link
Member Author

mahrud commented Nov 1, 2021

There should be no documentation node for "isEmpty" in the Polyhedra package

Why? There's no reason to disallow this, when with the changes in this PR both can exist at the same time. This was a necessary change a while back.

For instance, a common word like "quotient" or "euler" can mean different things in the context of different packages.

@DanGrayson
Copy link
Member

There should be no documentation node for "isEmpty" in the Polyhedra package

Why? There's no reason to disallow this, when with the changes in this PR both can exist at the same time. This was a necessary change a while back.

For instance, a common word like "quotient" or "euler" can mean different things in the context of different packages.

Because the symbol "isEmpty" is exported by the Core. Each package is allowed to document only its own symbols. it has nothing to do with the "meaning" of the symbol in the context of the package -- the package can document the methods it installs on the shared symbol, and that's sufficient.

Actually, it would be nice if we had a way to tell, after the fact, which package installed a given method function, so we don't allow the wrong package to document a method function.

@mahrud
Copy link
Member Author

mahrud commented Nov 1, 2021

Each package is allowed to document only its own symbols.

This has not been the case for a few versions, and the reason is that it made the documentation system more annoying and less effective.

@mahrud
Copy link
Member Author

mahrud commented Nov 1, 2021

Actually, it would be nice if we had a way to tell, after the fact, which package installed a given method function, so we don't allow the wrong package to document a method function.

Why? These are shared symbols. The whole point is to allow packages to share them, this include sharing the documentation!

@mahrud
Copy link
Member Author

mahrud commented Nov 1, 2021

the package can document the methods it installs on the shared symbol, and that's sufficient.

Here's the reason that this is insufficient:
Let's say package A exports a function (not even a method!) 'fancyFunc', and package B also exports 'fancyFunc', but these two packages don't depend on each other, so they both need to export it. Then at some point the dependencies change and one is loaded before the other, and suddenly there's an error, when 'fancyFunc' could do completely different things in package A and B and they may not even be methods to be distinguished by their input types. What are package writers supposed to do then?

This is just an unnecessary requirement that was removed for good reason, and there's no point in bringing it back.

Meanwhile, this PR is a simplification of the documentation system that happens to also fix this problem, so there's also no point here in discussing whether to bring back that requirement.

@DanGrayson
Copy link
Member

Each package is allowed to document only its own symbols.

This has not been the case for a few versions, and the reason is that it made the documentation system more annoying and less effective.

We'll have to put that back to the way it was before.

@DanGrayson
Copy link
Member

Actually, it would be nice if we had a way to tell, after the fact, which package installed a given method function, so we don't allow the wrong package to document a method function.

Why? These are shared symbols. The whole point is to allow packages to share them, this include sharing the documentation!

Yes, but the methods are not shared. If foo is a shared method and two packages install methods for foo(X,Y,Z), then that's a conflict that should result in an error.

@DanGrayson
Copy link
Member

the package can document the methods it installs on the shared symbol, and that's sufficient.

Here's the reason that this is insufficient: Let's say package A exports a function (not even a method!) 'fancyFunc', and package B also exports 'fancyFunc', but these two packages don't depend on each other, so they both need to export it. Then at some point the dependencies change and one is loaded before the other, and suddenly there's an error, when 'fancyFunc' could do completely different things in package A and B and they may not even be methods to be distinguished by their input types. What are package writers supposed to do then?

This is just an unnecessary requirement that was removed for good reason, and there's no point in bringing it back.

Meanwhile, this PR is a simplification of the documentation system that happens to also fix this problem, so there's also no point here in discussing whether to bring back that requirement.

If two packages export symbols with the same name, there should be no problem. Each package should be able to document its own symbols.

@mahrud
Copy link
Member Author

mahrud commented Nov 1, 2021

Yes, but the methods are not shared. If foo is a shared method and two packages install methods for foo(X,Y,Z), then that's a conflict that should result in an error.

Plenty of packages install the same exact method on types with the same name. This requirement limits what you can document using Macaulay2.

If two packages export symbols with the same name, there should be no problem. Each package should be able to document its own symbols.

With your arbitrary requirement this would result in an error.

@DanGrayson
Copy link
Member

Yes, but the methods are not shared. If foo is a shared method and two packages install methods for foo(X,Y,Z), then that's a conflict that should result in an error.

Plenty of packages install the same exact method on types with the same name. This requirement limits what you can document using Macaulay2.

I intended X, Y, and Z to be the same types, not types with the same names.

If two packages export symbols with the same name, there should be no problem. Each package should be able to document its own symbols.

With your arbitrary requirement this would result in an error.

No, my requirement was that each package should document only its own symbols. That's not determined by looking at the name of the symbol, but by looking at the symbol itself.

@mahrud
Copy link
Member Author

mahrud commented Nov 1, 2021

I intended X, Y, and Z to be the same types, not types with the same names.

And what happens if a package intends to overwrite a method installed by another package in order to improve it or cover more possibilities?

No, my requirement was that each package should document only its own symbols. That's not determined by looking at the name of the symbol, but by looking at the symbol itself.

What if one of those packages is Core or in one of the pre-loaded packages?


These are not hypothetical issues. They are all issues I've had to deal with repeatedly in multiple packages I've contributed to. Your resistance to see that this requirement is pointless is counterproductive because instead of trying to make documentation more difficult you could be making it easier by fixing any of #666, #1203, #1649, #1668, or just properly documenting how you designed symbols, symbol bodies, and frames so that someone else can fix those issues.

@DanGrayson
Copy link
Member

I intended X, Y, and Z to be the same types, not types with the same names.

And what happens if a package intends to overwrite a method installed by another package in order to improve it or cover more possibilities?

That shouldn't be done.

No, my requirement was that each package should document only its own symbols. That's not determined by looking at the name of the symbol, but by looking at the symbol itself.

What if one of those packages is Core or in one of the pre-loaded packages?

Okay, what if? Is there a problem?

@mahrud
Copy link
Member Author

mahrud commented Nov 7, 2021

@DanGrayson regardless of your disagreement on what nodes should be documentable by a package, this pull request fixes a bug in the internal fixup method and simplies the help method, so it should be merged.

@DanGrayson DanGrayson merged commit 7a4e964 into Macaulay2:development Nov 7, 2021
@mahrud mahrud deleted the quickfix/help branch November 7, 2021 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants