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

Teacher tool: added two simple variable validator rules #9859

Merged
merged 3 commits into from
Feb 8, 2024

Conversation

srietkerk
Copy link
Contributor

I added these just really quick to have a quick validator for variables_set and variables_get, as a variable validator will be important for rubrics for tutorials. I think they might need to be a bit more nuanced than this, but it's a start.

@srietkerk srietkerk requested a review from a team February 8, 2024 22:41
{
"validator": "blocksExist",
"blockCounts": {
"variables_set": 1
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to add an additional check here for variables_change, which will also work as a set in some cases (but keep threshold as 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to keep that as a separate check and wait for feedback on it. It makes sense that we assign a variable a value when the variable is created so there are less errors. However, allowing a variables_change to act as a setter in a validation scenario feels like a dangerous thing to allow. The act of distinctively setting a variable value is a pretty important learning point for computer science.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add a validator for variables_change, too, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I don't think we need to add a whole separate validator for it. Feel free to leave as-is for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Validating that the program changes the value of a variable is actually non-trivial, since both "change" or "set" could do it, but "set" would have to be called twice on the same variable to achieve the intuitive result, which might be tricky to check. I'd only go down that rabbit hole if we get feedback asking for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll leave as is, then. Will definitely want to revisit the "variable changed" validation in the future.

Copy link
Contributor

@thsparks thsparks left a comment

Choose a reason for hiding this comment

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

One nit and one suggestion, but LGTM on the whole!

@srietkerk srietkerk merged commit 5cc8bc3 into master Feb 8, 2024
6 checks passed
@srietkerk srietkerk deleted the srietkerk-variable-validation branch February 8, 2024 23:52
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.

3 participants