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

Dispose command #85

Merged
merged 2 commits into from
Nov 21, 2023
Merged

Conversation

wekesa360
Copy link
Contributor

This PR addresses the VirtualMachine Command Set adding the Dispose command to the python DSL

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 17, 2023
@@ -88,7 +88,7 @@ class Command:
id: int
out: Optional[Struct]
reply: Optional[Struct]
error: Set[ErrorType]
error: Set[ErrorType] | None
Copy link
Contributor

Choose a reason for hiding this comment

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

Set can be empty, so perhaps we don't need None here ?

  1. It's good not to have two ways to represent the same fact: None and set() are two ways to represent that there are no errors associated with a command. It would be unfortunate if some commands used the first and some the second way.
  2. it might lead to simpler code processing Commands if error cannot be None. For example, if we wanted to check whether an error code is valid then:
rc in command.error

is simpler than:

True if command.error is None else rc in command.error

At the same time, it's easy to check for there not being any error types:

if command.error:
    print("there are no errors allowed for this command")

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 have used an empty set: set () for the command with no error.

fix checkers

fix: union none to empty set
@michalgr michalgr merged commit 0c877f7 into facebookexperimental:main Nov 21, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants