-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: added google pubsub example #47
Conversation
thanks for your contribution. i'll check it later |
Better to add README.md to examples/README.md and explain that these examples implement some client/server models for this project. |
I was planning to do that after adding all the possible examples from my side so I can cover them in the examples/README.md file. But I can do that now if you'd like! |
How do you want to do it @WincerChan ? |
hi, please check the reviews, I left comments. @mechaadi |
@WincerChan you mean this? #47 (comment) |
@mechaadi no, i left comments in reviews |
@WincerChan i think you need to submit the reviewed comments, i can't see it right now. ![]() |
|
||
on_generated_subscription_name = 'projects/{project_id}/subscriptions/{sub}'.format( | ||
project_id=PROJECT_ID, | ||
sub='on_generated-sub', |
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.
sub name should be on_generate-sub
, not generated
@mechaadi
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.
It should be on_generated-sub, because the publisher is waiting for the on-generated event @WincerChan
@mechaadi oh sorry, my bad. i forgot to submit |
@WincerChan Happens, i just resolved both of your review comments, pelase check the same. Thanks! |
@mechaadi Thanks for your contribution, and please pay more attention to testing your code. |
I didn't see any issues in the code @WincerChan. There was just a spelling mistake in the readme file. The code was tested before I created the PR. What part of code are you referring to for testing in this PR? |
@mechaadi I follow the readme instructions to create the topics, but I got error when I was running code. After debugging, I found it was a mismatch of “on-generate(d)” between readme and code. |
@WincerChan Yeah as I replied to your "review comment" (#47 (comment)) as well, there was nothing wrong with the example code. You're absolutely right about the typo (which was fixed here #47 (comment)) in the README.md file but not about the code testing as it was functional from the beginning. Thanks for the review and pointing out the typo in README.md file tho. |
@mechaadi I get that the typo was in the README, but the fact remains: following the README caused the code to fail. That’s not just a doc issue — it means the code relies on exact input without validation or helpful errors. If the code breaks due to a simple mismatch like this, it’s a usability issue in the code too, not just the docs. |
@WincerChan Well, I don't really agree with your last comment. Because the topic names and subscription names can be anything you like. The example folder code has the topic and subscription names already defined and one of those topic names just didn't simply match with the README.md file because of a typo in the README.md file. It clearly means that there was a typo in the README.md file. The code and code logic was 100% correct without any bugs when you run it as it is in the example folder. You can say that the README.md doesn't correctly describe the code because of that typo, but saying that the code had "Errors" is simply incorrect. |
@mechaadi I understand that the code works correctly when using the predefined topic and subscription names. However, my point is that following the README instructions should not lead to runtime errors. The typo in the README directly caused an error for users who rely solely on it. While topic names can indeed be arbitrary, the documentation must accurately reflect the code. It’s important to ensure that both the code and its documentation provide a smooth experience for everyone. |
@WincerChan Exactly, the README instructions had a typo, the code was right. I agree that the "the documentation must accurately reflect the code" and that's because there was a typo in the README.md file. Your initial statement where you said "please pay more attention to testing your code" doesn't align with this comment of yours. The typo was in the README.md file, which means the code was tested and correct. I see an ambiguity in your statement but I dont want to argue on it anymore. Thanks for this awesome library and maintaining it. Goodbye :)) |
@mechaadi Let me crystallize the key takeaway to ensure we align on priorities: The ultimate truth in open source is this: If a user follows the documentation and encounters an error, the project has failed them — regardless of whether the root cause is a doc typo, incorrect code logic, or anything else. Yes, the code is technically correct if used with exact predefined names. But the fact that a single typo in documentation caused a runtime failure without clear guidance reveals an opportunity to improve the code's resilience. For example: adding validation for expected topics/subscriptions or creating resources if appropriate. Testing code should cover not just code logic, but also how users interact with the system end-to-end. I appreciate your work on this PR again. I'll lock this conversation. |
This contains the pubsub example, will be adding zeromq, fastapi and socketio once this is merged