Make --no-event the default for local invoke #1477
Conversation
If no explicit `--event` is passed in, the default assumption is that no event is desired. STDIN events must be explicit. This is a breaking change.
| ) | ||
| @click.option("--no-event", is_flag=True, default=False, help="Invoke Function with an empty event") | ||
| @click.option("--no-event", is_flag=True, default=True, help="DEPRECATED: By default no event is assumed.", hidden=True) |
There was a problem hiding this comment.
Do we have a plan to remove this option all together?
There was a problem hiding this comment.
I'm open to that, question is if it's worth breaking people who are already using it. But by making it hidden, we're more or less de facto deleting it. The next step would be to drop values on the floor if someone goes to the trouble of doing --no-no-event or similar.
There was a problem hiding this comment.
Can we actually add a line somewhere "DEPRECATED" onto stdout, before we start looking for values on stdin. Similair to how python2.7 was deprecated.
There was a problem hiding this comment.
You don't have --no-no-event defined. So customers shouldn't be doing that at least. (https://click.palletsprojects.com/en/7.x/options/#boolean-flags).
I am ok not deleting it fully. You marked it as DEPRECATED in the help text, which would indicate we would remove it at some point. Maybe the right thing to do is not to that at all, and allow people to explicitly define --no-event. Once this is released, I am not sure why people would type more but it really doesn't hurt anything. If we are planning on removing completely, I would like us to have a way to do that was all. This is a non-blocking comment but worth thinking about.
There was a problem hiding this comment.
I think if someone explicitly defines --no-event there is no harm done - we're basically making it a no-op regardless, and I believe the deprecation flag removes it from help text anyways.
So perhaps the best way forward is to just leave it in place, but have it rendered inert.
We don't want an exception, the test is invalid.
|
The documentation still says:
which is no longer true, and it took me a while to find that this breaking change was the reason I could no longer invoke via stdin without explicitly specifying Where is the best place to raise a ticket for updating the docs? |
If no explicit
--eventis passed in, the default assumption is that noevent is desired. STDIN events must be explicit.
This is a breaking change.
Issue #, if available:
Description of changes:
Checklist:
make prpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.