-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: support embellishments and non-punctuation delimiters #42
Conversation
packages/unified-latex-util-arguments/libs/gobble-single-argument.ts
Outdated
Show resolved
Hide resolved
packages/unified-latex-util-arguments/libs/gobble-single-argument.ts
Outdated
Show resolved
Hide resolved
packages/unified-latex-util-arguments/libs/gobble-single-argument.ts
Outdated
Show resolved
Hide resolved
packages/unified-latex-util-arguments/libs/gobble-single-argument.ts
Outdated
Show resolved
Hide resolved
packages/unified-latex-ctan/package/minted/libs/argument-parser.ts
Outdated
Show resolved
Hide resolved
Thanks for the contribution! It looks like you have some tests failing... Here are some thoughts:
That solution is not exactly elegant, though... |
Regarding re-using existing algorithm for
|
This function caches results. Don't mutate the returned AST! |
A possible idea for
e{xyz}
is that you keep a record of whetherx,y,z
has been captured. If one of them has been, you can re-call the function withy,z
(or whatever hasn't yet been captured). This could potentially result in a way to keepgobbleSingleArgument
returning a singleton
It seems doable, but then the caller of gobbleSingleArgument
should maintain some kind of state, about embellishment tokens we have matched until now, its index, and so on. gobbleSingleArgument
will need a 4th argument startPosInStartPos
, which would indicate the starting position inside the current node's content (if it is Ast.String
node). Maintaining state would be done inside the body of gobbleArguments
, but it doesn't look like the right place for that logic, because any serious logic for argspec is in gobbleSingleArgument
function body...
If you prefer to have its return type Ast.Arguments[] | null
, then I'll refactor that.
Tests
I guess it should be passing now. For some reason, tests in unified-latex-cli
fail due to timeout on my machine, so I can't test it but the rest of tests were passing on my end.
Besides, I have made some tests for until
argspecs skip
, because they were failing on my end in the current HEAD.
It appears that e{{_}} has the same effect as e{_}. e{{{_}}} triggers an error. This commit reflects this behavior in `gobbleSingleArgument` function.
I think the real issue with changing The mutation warning is just that: a warning against mutating. However, if you create a new I'm leaning more towards making |
I wasn't aware of that! Thank you for pointing it out. I've updated code to implement this behavior. Also I've fixed optionalToken and added a test for |
gobbleSingleArguments return values
FYI, I just merged #45 which switches the tests to vitest. They might run successfully on your computer now :-) |
|
packages/unified-latex-util-arguments/libs/gobble-single-argument.ts
Outdated
Show resolved
Hide resolved
packages/unified-latex-util-arguments/libs/gobble-single-argument.ts
Outdated
Show resolved
Hide resolved
packages/unified-latex-util-arguments/libs/gobble-single-argument.ts
Outdated
Show resolved
Hide resolved
packages/unified-latex-util-arguments/libs/gobble-single-argument.ts
Outdated
Show resolved
Hide resolved
packages/unified-latex-util-arguments/libs/gobble-single-argument.ts
Outdated
Show resolved
Hide resolved
packages/unified-latex-util-arguments/libs/gobble-single-argument.ts
Outdated
Show resolved
Hide resolved
packages/unified-latex-util-arguments/tests/attach-arguments-in-array.test.ts
Show resolved
Hide resolved
Thanks for this! I left a few comments. Also, please run Prettier on all modified files. I'd still like to move some of the complexity to |
I've added eslint and added Regarding refactoring |
Thanks for the linter updates! Let me know when this is ready for another review :-) |
Now gobbleSingleArgument gobbles single argument for each run, and may mutates its parameter argspec to remove gobbled arguments.
f318060
to
815024d
Compare
Refactored |
Also I think the current tsconfig is somewhat faulty,
fails with TS errors which shouldn't be an error, and |
Did you do a general build first? If you haven't built the dependencies, it will fail. I have |
Turned out that doing a general build fixes the issue with Regarding tsconfig, IMO, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really close!
@@ -18,7 +18,7 @@ export const argumentParser: ArgumentParser = (nodes, startPos) => { | |||
const { argument: optionalArg, nodesRemoved: optionalArgNodesRemoved } = | |||
gobbleSingleArgument(nodes, argSpecO, startPos); | |||
|
|||
let codeArg: Argument | null = null; | |||
let codeArg: Argument | Argument[] | null = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed anymore, right?
} = gobbleSingleArgument(nodes, OPTIONAL_ARGUMENT_ARG_SPEC, pos) as { | ||
argument: Argument; | ||
nodesRemoved: number; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is casting necessary?
type: "embellishment"; | ||
embellishmentTokens: string[]; | ||
embellishmentTokens: (Group | string)[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a group needed here? It seems like maybe the parser should be fixed to never return a group; e{xy}
should return embellishment tokens ["x", "y"]
. Or is the group for default args..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Group here is for e{{x}{y}}
. I agree with you that this should be fixed at pegjs level alongside with #46. But working on pegjs would broaden the scope of this pr too much, I'd prefer to fix it in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just merged #49 which should mean that groups are no longer returned as embellishmentTokens
.
const tokens = normalizeEmbellishmentTokens( | ||
argSpec.embellishmentTokens | ||
); | ||
argSpec.embellishmentTokens = tokens; // ArgSpec is mutated here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't mutate argSpec
in this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for example, are you OK with mutating it in gobbleArguments
function? What exactly is the goal you want by not mutating it here?
} | ||
); | ||
currPos = bracePos[1] + 1; | ||
matchNum = i + 1; // 1-based indices |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can move this complexity to gobbleArguments
. Since gobbleArguments
knows that it's an embellishment and openMark === <embellishment token>
, gobbleArguments
can put the arguments in the correct order once they're acquired.
Could you do a separate PR that shows what you're talking about in a couple of the workspaces? The Alternatively, importing |
I would prefer to recreate it on each call rather than mutate it.
|
Agreed. Can you just make it throw an error for now if it encounters a group and we can fix it in another PR
|
@theseanl Were you planning on finishing the last changes? |
I need to eventually get it done, because it is needed for my other works, but I've been rather busy and less motivated to refactor an already working code to meet a goalpost that keeps moving. Conceptual distinction of singleton and multiple objects feel artificial to me, I even think that such a tendency may be influenced by one's linguistic backgrounds.. I'd be ok if you take over and address the points you've mentioned in reviews. I'll try to find some time within a few weeks. |
Thank you for picking up the slack! |
Previously, it couldn't attach arguments for macros whose argspec
e{_}
,E{^}{default}
,r^_
This PR adds support for it. The second use-case may be marginal, I initially tried to make embellishments work, then realized that the same code would enable the second feature and unifies some codes.
Since a single embellishment argspec can represent multiple arguments, such as in
e{^_}
where each token^
and_
would represents 2 arguments,gobbleSingleArgument
s signature was changed to returnAst.Arguments | Ast.Arguments[] | null
.Currently, this just puts embellishment tokens as
openMark
in Ast. Would it be preferable to have a new separate property for embellishments?Next goal would be to support/test macro expansions for such arguments. Once it's done, I'd like to work on #24.
Please take a look and provide comments, and feel free to make your own changes!