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: Catalog Clean-Up #9965

Merged
merged 4 commits into from
Apr 15, 2024
Merged

Conversation

thsparks
Copy link
Contributor

@thsparks thsparks commented Apr 11, 2024

Now that parameterized criteria are supported in beta, I moved those out of test, then I reduced some of the duplication in our catalog.

Kept:

  • All function definitions have comments
  • At least one custom variable is set
  • At least one variable is accessed
  • At least ${count} comments
  • ${Block} used ${count} times
  • At least ${count} loops used
  • At least ${count} custom functions exist and get called
  • [Test] Ask Copilot: ${question}

Removed:

  • Two different kinds of loops used (in favor of At least ${count} loops used. They have slightly different functionality but I think it's ok to just have one)
  • A custom function exists and gets called (in favor of At least ${count} custom functions exist and get called)
  • At least one block has comments (in favor of At least ${count} comments)
  • A custom variable's value is set to a random number (seemed too specific)
  • The 'pick random' block is used (in favor of ${Block} used ${count} times)
  • Compare two numbers for equality (seemed too specific)

Related micro:bit change: microsoft/pxt-microbit#5592

While testing, I noticed a bug where parameters seem to be getting overwritten when we have multiple instances of the same catalog criteria, but I'll look into that separately.

"template": "A custom variable's value is set to a random number",
"id": "7AE7EA2A-3AC8-42DC-89DB-65E3AE157156",
"use": "block_comment_used",
"template": "At least ${count} comments",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this change, but wondering if we're still planning on updating this validator since it's just looking at block comments right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it'd be good to do. Not highest pri at the moment, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I get around to moving all the work items from our task board over to issues/bugs, I'll make sure we include that one.

@thsparks thsparks merged commit b1f61ad into master Apr 15, 2024
6 checks passed
@thsparks thsparks deleted the thsparks/teachertool/clean_catalog branch April 15, 2024 23:46
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