-
Notifications
You must be signed in to change notification settings - Fork 52
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
Refactor capabilities #1553
Refactor capabilities #1553
Conversation
src/Swarm/Language/Capability.hs
Outdated
| -- | Execute the 'Reprogram' command | ||
CReprogram | ||
| -- | Execute the `meet` and `meetAll` commands. | ||
CMeet | ||
| -- | Capability to introspect and see its own name | ||
CWhoami |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we get rid of CWhoami
and use CExec Whoami
? Or is there a special reason we need to keep it? If so, maybe we can document it better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, never mind, I see this capability is for both whoami
and self
commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but perhaps we can have two self explanatory capabilities or have self implied by whoami.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it should always be OK to (1) split out capabilities like this into individual capabilities for each command, then (2) edit any devices which currently provide the old combined capability so that they now provide both. The only reason not to do that before was to prevent a proliferation of lots of capabilities, but now that we have CExecute
that's not really an issue.
In fact, it's probably a good idea to just split all such capabilities since it gives more fine-grained control for scenario authors. For example, the time
capability used to allow running both time
and wait
commands, and we split it into timeabs
and timerel
so that we could have scenarios that allow you to run wait
but not time
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! That took a lot of effort, but our test coverage helped a lot. 😄
@xsebek what's the status of this PR? Can I help? |
@byorgey I seem to have forgotten about this; thanks for bringing it up. I think this is a worthwhile cleanup but can not say when I will be able to get around to rebasing this on the current main. |
* add `CExecute Const` * remove all capabilities for constants * update YAML to explicitly mention each capability for constant * closes #1548
d3a4299
to
e29e51b
Compare
Co-authored-by: Restyled.io <commits@restyled.io>
@kostmo @byorgey, I rebased this old PR and split capabilities to explicitly mention the I tried to use
|
@@ -71,7 +71,7 @@ data CapabilityCost e = CapabilityCost | |||
-- Otherwise, parse as a Map from capabilities to ingredients. | |||
instance (FromJSON e) => FromJSON (SingleEntityCapabilities e) where | |||
parseJSON x = | |||
simpleList <|> (Capabilities <$> costMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kostmo I swapped these to get better errors, as they only told me that the wrong capability is not an object.
I don't know if this can be improved much. Maybe the costly capabilities could be under a separate key.
Here is the updated cheatsheet: cabal run swarm-docs -O0 -- cheatsheet --capabilities Expand table
|
Oh, nice, with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, that did end up being a lot of work. One of those things that seems like it will be simple/straightforward and ends up being more work than you thought. Thanks for doing it though, I think this is a big improvement!
CExecute Const
generic-data
package to getEnum
andBounded
Capability
type to have a single constructor for common case of "capability to execute command X" #1548