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

Default Arguments #83

Merged
merged 10 commits into from
Feb 20, 2024
Merged

Default Arguments #83

merged 10 commits into from
Feb 20, 2024

Conversation

siefkenj
Copy link
Owner

@siefkenj siefkenj commented Feb 19, 2024

This PR adds some of the functionality from #62 but with a slightly different approach.

Default arguments are now stored on argument nodes if an argument is not provided. a defaultArg is always a string.

Since a default argument may contain LaTeX code, it will need to be parsed again before inserting into an expanded macro. For example \NewDocumentCommand{\xxx}{E{^}{{\color{red}x}}}{#1} would store the string "\color{red}x as the default argument. A call to parse would be needed in expandMacros

@siefkenj
Copy link
Owner Author

@theseanl I have incorporated some functionality from #62 . Let me know if you want to rework the macro expansion in #82 to be based off of this work.

@theseanl
Copy link
Contributor

In #62, where default argument support is already completely implemented, adding default arguments to AST as done in this PR is not even needed. expandMacros have all the information needed to expand default arguments, so what this PR does (adding default arguments to renderInfo) is useless. You would be better off working on improving #62. I am open to more reviews.

@siefkenj
Copy link
Owner Author

I see you kept the default arg information just for expansion :-).

I like the argspec grammar in this PR better. I will reimplement the macro expansion on top of this PR. The other parts of #62 are the improvements to gobbleSingleArgument. We can revisit those after expansion is done.

@siefkenj siefkenj merged commit 898ac25 into main Feb 20, 2024
1 check 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.

2 participants