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

Chane ML Events from enum to classes so they can be expanded in autogenerated file. #4

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

microbit-carlos
Copy link
Contributor

@microbit-carlos microbit-carlos commented Jun 5, 2024

The blocks end up looking the same:
image

But rather than having an enum and types, which might force us through MakeCode features that we know have some bugs (microsoft/pxt-microbit#5695) we can use classes, with each "Ml event" being an instance of the class. This way we can create multiple instances in different files, so we can start with the "None" event (we'll need to think of a better name) and add extra events in the editor extension autogenerated files with the model and the labels.

@microbit-robert I've only tested this locally, are you able to test it in your environments to confirm if it works?

//% block="Circle"
Circle = 3,
namespace mlrunner {
export namespace Action {
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've added a nested namespace to make the Javascript code more readable and, since this is user defined, it avoids accidental overshadowing of existing variables/functions in the top mlrunner namespace.

pxtextension.ts Outdated
@@ -35,7 +31,7 @@ namespace mlrunner {
}
const actionLabels = actions.map((action, i) => ({
name: action,
value: i + 1,
value: i + 2,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because "None" needs to have a non-zero value, the user labels start at value "2", so I assumed this needs to be increased, but I'm not able to test it.

Alternatively we could check if the events can take negative values and make None = -1, but this is easier.

pxtextension.ts Outdated
Comment on lines 74 to 80
}
// End simulator code.


//% fixedInstances
//% blockNamespace=mlrunner
class MlEvent {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why, but it didn't work if the class was defined inside the namespace, so had to break it out.

Update autogenerated code so None can be triggered from the sim.
@microbit-robert
Copy link
Collaborator

@microbit-carlos I've changed the actions so they are an array of MlEvent instances and moved it inside the Actions namespace. The autogenerated has also been updated so we always have the ability to simulate "None".

@microbit-robert microbit-robert self-requested a review June 5, 2024 11:36
Copy link
Collaborator

@microbit-robert microbit-robert left a comment

Choose a reason for hiding this comment

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

LGTM. I've tested this, including with the changing and addition of action names by uploading different data.json files via the editor extension. The class method handles these changes very well unlike the enum version which resulted in errors.

@microbit-robert microbit-robert merged commit 77951b0 into main Jun 5, 2024
@microbit-robert microbit-robert deleted the ml-events-classes branch June 5, 2024 11:38
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.

None yet

2 participants