-
Notifications
You must be signed in to change notification settings - Fork 4
[LIB-701] BUGFIX Pull request #194
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
Conversation
…o Script manager;
|
@forgems Mr Świderski - this PR corrects all bugs that you reported - please take a look and share your thoughts. The sooner the better! ;) |
syncano/models/manager.py
Outdated
| @clone | ||
| def get_runtimes(self): | ||
| """ | ||
| Method which get available runtimes from CORE; |
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.
from Syncano API
syncano/models/manager.py
Outdated
| return self.model._meta.resolve_endpoint(self.endpoint, defaults), defaults | ||
|
|
||
|
|
||
| class RuntimeChoices(object): |
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.
IMHO it's not needed. You just removed the validator from runtime_name, so why bother with this ?
Also why not simply return a list ? When you set attributes for an object you have to do dir(object) instead just print a list.
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.
The list was in first version. Thought that would be nicer in such 'container'; the usage:
RUNTIMES = Script.please.get_runtimes()
# and later
Script.please.create(runtime_name=RUNTIMES.PHP, ...)
# rather than:
RUNTIMES = Script.please.get_runtimes()
# and later
Script.please.create(runtime_name=RUNTIMES[2], ...)
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.
As I understood, you wanted to implement something that reasembles metaenum, but your solution doesn't work like this, because you set attributes on class instance, not on a class itself.
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.
So why simply not "Script.runtime_name.choices" then? You could define a field with @cached_property choices, and make a request from there.
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 it a big deal here? :)
That this is an class instance and not class itself?
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.
Yep - thinkin bout that :) That user must do some inspection.
How do you want to provide constants, if runtime_names can change? dynamically?
Do you have any idea?
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.
And as for solution with list - user must do some inspection too! :)
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.
print(choices) vs print(dir(choices)) is shorter ;)
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.
Few more comments and I will remove this get_runtimes functionality ;)
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.
|
lgtm |
No description provided.