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

Move grammar generation to Style package #3662

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

d-torrance
Copy link
Member

As suggested by @mahrud in #3298 (comment)), we move the generateGrammar function from a build script to a method exported by the Style packagee.

@mahrud
Copy link
Member

mahrud commented Feb 20, 2025

I probably don't have time to review this in the short term.

Comment on lines 18 to 20
-- TODO: Move these two elsewhere:
Function and Function := (f, g) -> s -> f s and g s
Function or Function := (f, g) -> s -> f s or g s
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to actually move these to Core while you're moving them? Maybe also add not Function and Function xor Function. Especially the former would be super useful for things like this:

i1 : not Function := f -> x -> not f x

i2 : select({0,1,0,2,0,3}, not zero)

o2 = {1, 2, 3}

Copy link
Member Author

Choose a reason for hiding this comment

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

Might as well!

Comment on lines 61 to 62
if #bad > 0 then error(
"encountered symbol(s) that are not alphanumeric or have less than 2 characters:",
concatenate apply(bad, (name, symb) -> {
"\n\t",
-* TODO: symbolLocation name, ": here is the first use of ", *-
toString name}));
Copy link
Member

Choose a reason for hiding this comment

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

I recently learned something super cool about how error prints symbols which I didn't know when I wrote this:

i7 : error splice("encountered symbol(s) that are not alphanumeric or have less than 2 characters:",
     toSequence between_" " {symbol Module, symbol print})
stdio:13:5:(3): error: encountered symbol(s) that are not alphanumeric or have less than 2 characters:'Module' 'print'
../linuxbrew/.linuxbrew/share/Macaulay2/Core/expressions.m2:1234:42-1234:48: here is the first use of 'Module'
../linuxbrew/.linuxbrew/share/Macaulay2/Core/expressions.m2:1158:0-1158:5: here is the first use of 'print'

Copy link
Member Author

Choose a reason for hiding this comment

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

That's neat!

I'm a little confused at this whole error message, though. The symbols list appears to be made up of two things: the list of all packages and everything from the dictionaries in the pre-loaded packages. We already call select with okay on the latter, so we know that none of those will end up in bad. Why not just do the same thing on the former?

And also, these packages are always paired up with the Core symbol (I guess so that when we call is on them, we know that they're packages), so even if one of them ends up in bad and we modify the error message to print the symbol location, it will just be the location of Core.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure. Maybe experiment with not selecting on line 51 and see if anything breaks?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are tons of non-alphanumeric symbols (mathematical operators, all the Package$foo ones) that were getting filtered out by select.

The only elements of this list we weren't selecting out with okay are the package names, and we know that they'll be alphanumeric since newPackage raises an error when they're not. I just pushed a commit removing this check.

cachedSymbols.Function = first \ select(symbols, isFunction);
cachedSymbols.Constant = sort join(
first \ select(symbols, isConst),
{"Node", "Item", "Example", "CannedExample", "Pre", "Code", "Tree",
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind adding a comment close to this, and also somewhere in SimpleDoc, to keep these in sync?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason we hardcode these here instead of just exporting them from SimpleDoc and letting them get pickedup with all the other symbols? (I suppose Node is a class, so it would get a different color -- is that the main reason?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I just pushed a commit exporting these docstring symbols from SimpleDoc so we can stop hard-coding them here. I also renamed the Node class to DocNode so Node would still be the same color as the others.

Copy link
Member

@mahrud mahrud left a comment

Choose a reason for hiding this comment

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

I haven't checked this myself, but if the output is identical, this seems fine.
I left a few comments that would be good to implement, though I realize that those are just things you moved, not added.

The "symbols" list is the union of two sets:
* {(pkgname, Core) : pkgname is the name of a package}
* {(name, symb) : key/value pair from a pre-loaded package dictionary}

pkgname will always be okay (we check for this in "newPackage", and we
already ran "select" w/ the okay function on the second set
@d-torrance d-torrance requested a review from mahrud February 25, 2025 12:08
Comment on lines 38 to 45
"CannedExample",
"Code",
"Example",
"Item",
"Node",
"Pre",
"Synopsis",
"Tree",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these should be exported by SimpleDoc. Some of them are very common words and could lead to random frustrations.

Copy link
Member Author

Choose a reason for hiding this comment

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

And case in point, we got the following build failure from the Varieties::tangentSheaf(ProjectiveVariety) example :)

i5 : Node = Proj(QQ[a,b,c]/ideal(b^2*c-a^2*(a+c)))
stdio:5:11:(3): error: assignment to protected global variable 'Node', originally defined at stdio:5:0:(3):

I'll restore the original behavior but add some comments. The annoying this is that many of the SimpleDoc docstring symbols are in fact symbols defined in Core, but not these few.

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.

2 participants