[pigeon] reved pigeon, tweaked contributors changes before publish

[gaaclarke]

• updated run_tests.sh
• removed the ability to accept null callbacks in java code (unpublished change)
• switched Map's java type to Map<Object, Object>, it was previously Map<String, Object> in an unpublished PR.

Pre-launch Checklist

• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• I updated CHANGELOG.md to add a description of the change.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[jmagman]

removed the ability to accept null callbacks in java code (unpublished change)

Was that causing problems?

[gaaclarke]

Was that causing problems?

No, it's just I accepted it in a PR but wasn't sure that I actually wanted the API change. I don't like hiding an asynchronous operation by making it look like a synchronous one. I'm more ok with accepting null as the parameter, but again it isn't a huge deal to just supply a lambda that does nothing. Then it is clear at the callsite that it is an asynchronous operation.

doSomething(input, (result) -> {});
// vsdoSomething(input, null);
// vsdoSomething(input);

I don't see a problem forcing people to do the first option because it's more clear it's asynchronous.

[gaaclarke]

cc @matejdro I'm pulling out the null arguments for callbacks in java. When I approved the PR I was fine with the code, I didn't put much consideration to the change in the API. See my previous comment for the justification.

[matejdro]

I would argue against that. Void operations are usually fire-and-forget. Requiring null just pollutes the code.

[gaaclarke]

I would argue against that. Void operations are usually fire-and-forget. Requiring null just pollutes the code.

Even for void returning functions the callback has meaning, it means the operation has completed. People need to consider if they care about that.

[gaaclarke]

(Ideally the API would be using futures, then this would be a bit more clear but I decided to use lambdas to keep the API similar to platform channels)

[matejdro]

I guess it depends on the use case. On our current project (https://github.com/pebble-dev), vast majority of the pigeon methods just send various events from native to dart. Native does not care if event was delivered or not.

I think that out of dozens of methods, only 1-2 actually require the callback. So this null requirement causes pretty big nuisance in the code base.

[gaaclarke]

I guess it depends on the use case. On our current project (https://github.com/pebble-dev), vast majority of the pigeon methods just send various events from native to dart. Native does not care if event was delivered or not.

I think that out of dozens of methods, only 1-2 actually require the callback. So this null requirement causes pretty big nuisance in the code base.

But you can just say doSomething(input, (result) -> {}); instead of doSomething(input);, right? That's not a huge deal and if you guys run into some sort of bug from a temporal dependency where one operation should be completed before another is started, you'll have a hint in the code about that temporal dependency. Hiding asynchronous operations usually bites people in the end.

For example

foo(input);
bar(input);

should actually be:

foo(input, (fooResult) -> {
bar(input, (barResult) -> {});
});

[matejdro]

I completely understand. But it's still polluting code to use doSomething(input, (result) -> {}); instead of just calling doSomething(input) when you are aware that result can be discarded.

[gaaclarke]

I completely understand. But it's still polluting code to use doSomething(input, (result) -> {}); instead of just calling doSomething(input) when you are aware that result can be discarded.

Thanks matejdro for the feedback, truly. What you see as pollution I see as an insurance policy against temporal bugs. If you want you could always make your own function that throws away the lambda:

staticvoiddoSomething(Apiapi, Inputinput) {
api.doSomething(input, (result) -> {});
}

[matejdro]

I agree that I could make my own function (and I did in some cases. But it gets tiring wrapping every single pigeon in my own function.

[jmagman]

I'm not a Java developer so I don't have an intuitive sense of what's idiomatic, so I looked for an example.

AsynchronousFileChannel.write() is similar in spirit to doSomething(input, (result) -> {});. We already know what data is being sent over the channel/being written to a file, but do we care when it's done?

There's an async API Future<Integer> write(ByteBuffer src, long position) and an abstract <A> void write(ByteBuffer src, long position, A attachment, CompletionHandler<Integer,? super A> handler) with a handler. Our void example is closest to the latter. There's no abstract <A> void write(ByteBuffer src, long position, A attachment) convenience method that doesn't require completion handler.

However I definitely don't want to get into a stylistic war here.

What you see as pollution I see as an insurance policy against temporal bugs.

I'm with @gaaclarke here, I'd rather it be explicit than convenient. LGTM