Removing superfluous refcap ceremony from example #4169
Conversation
Where:
```pony
class ProcessClient is ProcessNotify
⋮
new iso create(env: Env) =>
⋮
```
The following ceremony in Main isn't required:
```pony
let client = ProcessClient(env)
let notifier: ProcessNotify iso = consume client
```
It can more clearly be expressed as:
```pony
let client: ProcessClient iso = ProcessClient(env)
```
ponylang-main
added
the
discuss during sync
| @@ -21,8 +21,7 @@ use "files" | |||
| actor Main | |||
| new create(env: Env) => | |||
| // create a notifier | |||
| let client = ProcessClient(env) | |||
| let notifier: ProcessNotify iso = consume client | |||
| let client: ProcessClient iso = ProcessClient(env) | |||
There was a problem hiding this comment.
well, why bother with the : ProcessClient iso?
that can go as well.
There was a problem hiding this comment.
I kept it for two reasons:
- The original did it.
- If someone is reading the example then they have to look in one of three places to realize that ProcessMonitor takes a ProcessNotify iso.
- ProcessClient.create()
- The ProcessMonitor.create function signature, or
- The ProcessMonitor callsite in the example (and infer that the consume likely means it was originally an iso).
For somebody unfamiliar with the package - I wanted that to be explicit.
If you'd still rather lose the definition I can lose it, but that is why I chose to be explicit in this case.
|
I think the commit comment for this should be changed. This doesnt at the moment remove any refcap ceremony. It removes an unneeded variable. With my noted change, it could remove an unrequired type annotation and an not needed variable. Either way, the commit message (and PR title) should be adjusted. |
| @@ -32,7 +31,7 @@ actor Main | |||
| // create a ProcessMonitor and spawn the child process | |||
| let sp_auth = StartProcessAuth(env.root) | |||
| let bp_auth = ApplyReleaseBackpressureAuth(env.root) | |||
| let pm: ProcessMonitor = ProcessMonitor(sp_auth, bp_auth, consume notifier, | |||
| let pm: ProcessMonitor = ProcessMonitor(sp_auth, bp_auth, consume client, | |||
There was a problem hiding this comment.
also, not sure why we are doing : ProcessMonitor here. We have some annotations on some and none on others. I think we should either have none or all for the example, not this in between state.
There was a problem hiding this comment.
Well, you know that my preference is to annotate everywhere - (especially in docs where the purpose is to communicate as easily as possible).
The pony style-guide doesn't mention omitting types and refcaps on variable definitions so I didn't consider that to be an overriding concern...
There was a problem hiding this comment.
Then I would say, annotations for all here. And updates to commit message (if you update first message here and pr title, they can be used on squash).
SeanTAllen
removed
the
discuss during sync
|
@redvers I'm going to close this as it hasn't gone anywhere. |
ponylang-main
added
discuss during sync
Where:
The following ceremony in Main isn't required:
It can more clearly be expressed as: