Skip to content

Conversation

@opalczynski
Copy link
Contributor

No description provided.

@opalczynski
Copy link
Contributor Author

@forgems Mr Świderski - this PR corrects all bugs that you reported - please take a look and share your thoughts. The sooner the better! ;)

@clone
def get_runtimes(self):
"""
Method which get available runtimes from CORE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Syncano API

return self.model._meta.resolve_endpoint(self.endpoint, defaults), defaults


class RuntimeChoices(object):
Copy link
Contributor

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.

Copy link
Contributor Author

@opalczynski opalczynski May 12, 2016

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], ...)

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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! :)

Copy link
Contributor

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 ;)

Copy link
Contributor Author

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 ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dancio @forgems So i removed this method and palce normal constants - what do ya think guys? I need to merge it and release LIB tomorrow to end first phase of parse task :)

@dancio
Copy link
Contributor

dancio commented May 12, 2016

lgtm

@opalczynski opalczynski merged commit 6b60640 into develop May 12, 2016
@opalczynski opalczynski deleted the LIB-701 branch May 12, 2016 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants