Skip to content

Conversation

@TannerGabriel
Copy link
Contributor

Closes #9

Copy link
Collaborator

@mark-friedman mark-friedman left a comment

Choose a reason for hiding this comment

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

@TannerGabriel Thanks for taking this on! How did you test your changes?

@TannerGabriel
Copy link
Contributor Author

I tried multiple configurations with the getters and setters, logging and comparing block_ and sourceBlock_ and comparing the results of the dropdown. If I should do some further testing, let me know.

@mark-friedman
Copy link
Collaborator

I tried multiple configurations with the getters and setters, logging and comparing block_ and sourceBlock_ and comparing the results of the dropdown. If I should do some further testing, let me know.

When testing various configuration, here are some of the things that I manually test, in the context of some reasonably deeply nested set of definitional and other random blocks. Make sure to have some of the variable definitions shadow other variable definitions (i.e. have the same name). Also include a global variable definition in the workspace:

  • get and set blocks show the correct set of variable names in their drop downs at each point in the nested structure.
  • Renaming variables correctly renames the get and set blocks within, and only within, their scope.
  • Moving a get or set block out of it's scope shows a warning and moving the block back into an appropriate scope removes the warning.

You should also run the unit tests via npn run test

@TannerGabriel
Copy link
Contributor Author

Thank you for the input. I will manually test a few more configurations tomorrow and get back to you after that.

The unit tests are all passing. Do you think it would be useful to add a small GitHub Action that automatically runs the unit tests for each PR?

@mark-friedman
Copy link
Collaborator

Do you think it would be useful to add a small GitHub Action that automatically runs the unit tests for each PR?

That would be great! Thx.

@TannerGabriel
Copy link
Contributor Author

Sure, I will create a separate PR for that tomorrow.

@TannerGabriel
Copy link
Contributor Author

Created the PR for the GitHub action #72. Also, manually tested the additional cases you mentioned, which seem to be working fine.

@mark-friedman mark-friedman merged commit 230f173 into mit-cml:main Aug 10, 2025
@TannerGabriel TannerGabriel deleted the feat/remove-block-field branch September 8, 2025 14:33
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.

Maybe remove FieldLexicalVariable.block_

2 participants