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.

Sign up for GitHub

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

Jump to bottom

DYN-7112: Fix DS parser to error early on finding import statements in code block nodes #15288

Merged
merged 17 commits into from
Nov 8, 2024
Merged

DYN-7112: Fix DS parser to error early on finding import statements in code block nodes #15288

merged 17 commits into from
Nov 8, 2024

Conversation

aparajit-pratap
Copy link
Contributor

Purpose

Fix DS parser to error early on finding import statements in code block nodes

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

Fix parser to error early on finding import statements in code block nodes

@mjkkirschner
Copy link
Member

mjkkirschner commented Jun 5, 2024 •
edited
Loading

any way to unit test this so no one breaks it later?

@aparajit-pratap aparajit-pratap changed the title Fix DS parser to error early on finding import statements in code block nodes [WIP] Fix DS parser to error early on finding import statements in code block nodes Jun 6, 2024
@aparajit-pratap
Copy link
Contributor Author

any way to unit test this so no one breaks it later?

Sure, will add one.

I also need to fix failing tests as they run under the assumption that IsParsingCodeBlockNode is true while at the same time containing import statements. As this is not a real Dynamo use-case, the tests/test framework needs some adjustment as to not make these assumptions.

@aparajit-pratap aparajit-pratap changed the title [WIP] Fix DS parser to error early on finding import statements in code block nodes DYN-7112: [WIP] Fix DS parser to error early on finding import statements in code block nodes Nov 5, 2024
github-actions[bot]
github-actions bot reviewed Nov 5, 2024
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7112

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 6, 2024 •
edited
Loading

UI Smoke Tests

Test: success. 11 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests

@aparajit-pratap aparajit-pratap changed the title DYN-7112: [WIP] Fix DS parser to error early on finding import statements in code block nodes DYN-7112: Fix DS parser to error early on finding import statements in code block nodes Nov 7, 2024
mjkkirschner
mjkkirschner reviewed Nov 7, 2024
View reviewed changes
src/Engine/ProtoCore/Utils/ParserUtils.cs Outdated
IsParsingCodeBlockNode = true,
IsParsingPreloadedAssembly = false
IsParsingPreloadedAssembly = false,
IsParsingInTestMode = true,
Copy link
Member

@mjkkirschner mjkkirschner Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why should this be true here? Isn't this a general utility function not a test helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually a test helper. I'll see if I can refactor it or rename it so that's clear.

aparajit-pratap
aparajit-pratap commented Nov 8, 2024
View reviewed changes
test/Engine/ProtoTest/FFITests/CSFFIDispose.cs
var cbn = ProtoCore.Utils.ParserUtils.Parse(code);
var subtree = null == cbn ? new Subtree(null, guid) : new Subtree(cbn.Body, guid);
return subtree;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused

aparajit-pratap
aparajit-pratap commented Nov 8, 2024
View reviewed changes
test/Engine/ProtoTest/LiveRunnerTests/ChangeSetComputerTests.cs
var cbn = ProtoCore.Utils.ParserUtils.Parse(code);
var subtree = null == cbn ? new Subtree(null, guid) : new Subtree(cbn.Body, guid);
return subtree;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused

@aparajit-pratap aparajit-pratap merged commit f1e43f5 into DynamoDS:master Nov 8, 2024
24 checks passed
@aparajit-pratap aparajit-pratap deleted the fixParser branch November 8, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@mjkkirschner mjkkirschner mjkkirschner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aparajit-pratap @mjkkirschner