-
Notifications
You must be signed in to change notification settings - Fork 4
[LIB-783] - Correct endpoints check methods #213
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
… Meta class, apply changes to ScriptEndpoint model
| raise SyncanoValidationError('Method allowed only on existing model.') | ||
|
|
||
| properties = self.get_endpoint_data() | ||
| endpoint = self._meta.resolve_endpoint('run', properties) |
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.
In:
def resolve_endpoint(self, name, properties):
endpoint = self.get_endpoint(name)
for name in endpoint['properties']:
if name not in properties:
raise SyncanoValueError('Request property "{0}" is required.'.format(name))
return endpoint['path'].format(**properties)
is get_endpoint which is:
def get_endpoint(self, name):
if name not in self.endpoints:
raise SyncanoValueError('Invalid path name: "{0}".'.format(name))
return self.endpoints[name]
Maybe we should add check in resolve_endpoint for that? Would be much cleaner. What do you think?
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.
@opalczynski Fine :)
|
@opalczynski Please check this now :) |
syncano/models/options.py
Outdated
|
|
||
| return endpoint['path'].format(**properties) | ||
|
|
||
| def is_http_method_available_for_endpoint(self, http_method_name, endpoint_name): |
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_http_method_available_for_endpoint -> is_http_method_available is sufficient
|
Please add some tests; |
|
Yes, tests will be there. I wanted to know if the approach is correct. |
|
@opalczynski it's ready :) |
syncano/models/channels.py
Outdated
| def poll(self, room=None, last_id=None, callback=None, error=None, timeout=None): | ||
| properties = self.get_endpoint_data() | ||
| endpoint = self._meta.resolve_endpoint('poll', properties) | ||
| endpoint = self._meta.resolve_endpoint('poll', properties, 'GET') |
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.
:P you're using in all previous one the pattern:
http_method = 'GET'
self._meta.resolve(..., http_method)
maybe just add:
endpoint = self._meta.resolve_endpoint('poll', properties, http_method='GET')
or make http_method='GET' as default
|
This is nice. One small comment. LGTM. |
|
++ |
@opalczynski Here we go with PR. Please check if it's fine. If yes then I will adapt other models using the same approach.