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

Added iteration scope for js statements #2144

Merged
merged 2 commits into from
Dec 18, 2023
Merged

Added iteration scope for js statements #2144

merged 2 commits into from
Dec 18, 2023

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Dec 15, 2023

Checklist

  • I have added tests
  • [-] I have updated the docs and cheatsheet
  • [-] I have not broken the cheatsheet

@AndreasArvidsson AndreasArvidsson changed the title Added iteration scope for job script statements Added iteration scope for js statements Dec 15, 2023
@@ -47,8 +45,14 @@ export function getScopeTestPaths() {
.filter((p) => p.endsWith(".scope"))
.map((p) => ({
path: p,
name: path.relative(relativeDir, p.substring(0, p.lastIndexOf("."))),
name: pathToName(relativeDir, p),
Copy link
Member Author

@AndreasArvidsson AndreasArvidsson Dec 15, 2023

Choose a reason for hiding this comment

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

Had forgot to update the name of scope test cases

@@ -0,0 +1,33 @@
// Start
Copy link
Member Author

Choose a reason for hiding this comment

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

Included to make it very obvious that it's not the function, but the entire document

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

looks good with minor comment

ifStatement: supported,
regularExpression: supported,
switchStatementSubject: supported,
fieldAccess: supported,

statement: supported,
"statement.iteration": supported,
Copy link
Member

Choose a reason for hiding this comment

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

This will absolutely get split up, and probably sooner rather than later. Should we do that now? Eg statement.iteration.file and statement.iteration.block? Tho block tends to mean something very different in our code base 🤔

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 was thinking about that, but there is no way to do separate scopes fixtures for that, both files will have 2 scopes I'm afraid.

Block is just the spoken form. We do call it paragraph as an id. I'm fine with block in this context.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about that, but there is no way to do separate scopes fixtures for that, both files will have 2 scopes I'm afraid.

Yeah I'm not sure of a way around that. But I still think making it super explicit what the iteration scopes should be is preferred

Block is just the spoken form. We do call it paragraph as an id. I'm fine with block in this context.

Sounds good

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok will split it!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

👍

@pokey pokey added this pull request to the merge queue Dec 18, 2023
Merged via the queue into main with commit fff171f Dec 18, 2023
13 checks passed
@pokey pokey deleted the jsStatementIteration branch December 18, 2023 18:20
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