-
Notifications
You must be signed in to change notification settings - Fork 71
Conversation
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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SKairinos)
game_frontend/src/pyodide/syntax.ts
line 93 at r1 (raw file):
const fromImports: Record<string, Set<string>> = {}; for (const match of code.matchAll(pattern)) {
Do you need this for loop? Could we do something like this? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/matchAll
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SKairinos)
game_frontend/src/pyodide/syntax.ts
line 11 at r1 (raw file):
lineStart: boolean captureName: boolean captureArgs: boolean
Are these ever true at any point? If not, do we need them as args?
Code quote:
lineStart: boolean
captureName: boolean
captureArgs: boolean
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faucomte97)
game_frontend/src/pyodide/syntax.ts
line 11 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Are these ever true at any point? If not, do we need them as args?
not currently. I made them as args in a callable because if in the future we ever need to match a function signature, I would like us to reuse this rather than repeating pattern with slick tweaks each time.
game_frontend/src/pyodide/syntax.ts
line 93 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Do you need this for loop? Could we do something like this? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/matchAll
yes, it's needed because we process each match differently depending on the type of match. see the further processing inside the enclosed if/else statements
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1838 +/- ##
==========================================
+ Coverage 70.85% 70.93% +0.07%
==========================================
Files 192 193 +1
Lines 3991 4039 +48
Branches 272 283 +11
==========================================
+ Hits 2828 2865 +37
- Misses 1134 1145 +11
Partials 29 29
|
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)
This change is