-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
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.
Pulled and tested. The change works as expected. But the warning is separated from the other existing ZDW
warnings:
ZDW
warnings are generated by the backend server. For users to get a nice single consolidated view of all the errors and warnings, maybe it's better to check the key collision in the backend? This will require us to add a new ZDW
warning in the doc, though.
So it turns out that's way tricker than it sounds! The collision detection has to happen during the code -> definition step. The nice output shown above is generated in CLI, which interfaces with core (and the code itself) via the That is to say, the raw code never leaves the
So that's the long answer! Let me know what you think about it |
@xavdid you're right. By the time the app definition hits the server, the collision is gone and is impossible to detect. I'd vote for 5. But we probably want to add a TODO comment saying that this will be changed to throw an error instead of a warning output. Another question: Refer to the screenshot above. Why is the warning message printed twice? Can we fix it? |
Sweet, i've added notes in b585611. As for printing twice, that also can't be fixed without a lot of refactoring. Since the check (and printing) happens whenever Normally I'd want to fix that, but since it's going to be an error later, being extra loud about it (for the few people who will have this issue) isn't really a bad thing. Throwing an error instead of printing will (sort of) fix this, as the message will be shown once and the process will quit. |
@xavdid sounds fair. 👍 |
mirrors to PDE-35 and fixes zapier/zapier-platform-cli#41
Since resources auto-generate keys for their new triggers/etc, we should flag if there's a collision (which will cause confusing behavior).
Notes:
createAppTester
. We could potentially fix this by lettingcreateLambdaHandler
know whether or not it's being used for a test or something else, but I think the louder the error is, the better.