-
Notifications
You must be signed in to change notification settings - Fork 22
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
Custom usage counter example and integration test #611
Conversation
This reverts commit fce8c2e.
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.
added a few comments - plus, there should be some explanation (in the beginning of te source maybe? or in an accompanying README.md file) what the custom counter is and how it is increased so that the user can understand what's happening ... without some explanation, the whole example is very opaque...
plus, ideally, there should be a comment on how to run the example yourself, without goth
debug_market_api=True, | ||
debug_payment_api=True, | ||
) | ||
asyncio.run(main(args.running_time, args.subnet_tag, args.driver, args.network)) |
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.
Is there any reason why we don't do if __name__ == '__main__'
here?
Also, what with all this try/except, task.cancel()
etc that we have in most examples?
I think this might be quite confusing for the reader who already knows & understands previous examples.
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.
Is this necessary if we are not importing this file? It's just an example that is meant to be run directly, it even starts with #!/usr/bin/env python3
. I wanted this example to be as simple as possible, handling NoPaymentAccountError
and KeyboardInterrupt
would make it more complicated. It's not great when most of the code in the example is boilerplate.
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.
Someone might want to import main
from this example and use it in some other way. Why forbid them? It might make "playing with an example" easier.
I think this if __name__ ...
is just a good standard, although I won't insist.
About NoPaymentAccountError/KeyboardInterrupt etc: I definitely agree this should be simplified, but I'd rather have the same in all examples (except hello world) and then fix it in all examples at the same time. But I won't insist here either : )
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.
I added if __name__ == "__main__":
(9b238e6)
BTW: this example does not run indefinitely, so Ctrl+C wouldn't be typically pressed:
while datetime.now() < start_time + timedelta(seconds=self._running_time_sec):
...
parser.add_argument(
"--running-time",
default=30,
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.
more or less looks good ...
added two more comments, it would be good @filipgolem if you could still include them
Done. |
A matching custom runtime:
https://github.com/golemfactory/ya-runtime-sdk/pull/16/files
presets.json
: