Fix env viz import issues #1451
Conversation
Pull Request Test Coverage Report for Build 5677180723
💛 - Coveralls |
src/ec-evaluator/interpreter.ts
Outdated
| command = agenda.peek() | ||
| steps += 1 | ||
| // If command is an ImportDeclaration, don't add unnecessary step | ||
| if (!isImportDeclaration) { |
There was a problem hiding this comment.
I agree that this seems a bit hacky, and also feels a bit like its going against the concept of the agenda. Another problem with this is that at step 2 (in the screenshot) the user thinks that the import statement is being evaluated, but in step 3, another command is evaluated instead.
There was a problem hiding this comment.
Suggestion: In 'Program' when the block is decomposed into individual statements and pushed onto the agenda, don't push any import statements since the imports are already evaluated in 'Program'. (I would think modifying the 'handleSequence' function appropriately would work best). Then we wouldn't need this hack since our evaluator won't encounter any import statements.
There was a problem hiding this comment.
Suggestion: In 'Program' when the block is decomposed into individual statements and pushed onto the agenda, don't push any import statements since the imports are already evaluated in 'Program'. (I would think modifying the 'handleSequence' function appropriately would work best). Then we wouldn't need this hack since our evaluator won't encounter any import statements.
this means that the import statements won't be in the agenda at all?
There was a problem hiding this comment.
Yep! The user can see the import statements initially in the program block, but since the import statements are evaluated in the program block itself it doesn't seem to make sense to continue to push the import statements on the agenda since they do nothing and only add to confusion.
Do you agree prof? @martin-henz
There was a problem hiding this comment.
Yes, I agree. The handling of the program needs to create the program environment which will include the imported names and the newly declared names. The imported names can be immediately assigned to their implementation without the need of executing a step in the machine.
There was a problem hiding this comment.
okay, i have made the necessary changes to the handleSequence function so the import statements are only in the program block but not on the agenda
martin-henz
had a problem deploying
to
publish-npm
GitHub Actions
Failure
ImportDeclarations are now handled in the Program evaluator in cmdEvaluators so that all toplevel names are shown right away in the program environment - Fixes #1450
ImportDeclarations are now non value producing - Fixes #1448 (POP issue)
Added a condition in runECEMachine to check if the command is an ImportDeclaration, then the current step is not incremented - Fixes #1448 (redundant steps issue but this feels quite hackish? )
