-
Notifications
You must be signed in to change notification settings - Fork 339
Adding support for TensorFlow 2.x #372
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
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.
Looks great. Just a couple of nits.
tests/test_mlkit.py
Outdated
def _url(project_id, model_id): | ||
def _update_url(project_id, model_id): | ||
update_url = 'projects/{0}/models/{1}?updateMask=state.published' | ||
return BASE_URL + update_url.format(project_id, model_id) |
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.
Nit: Consider doing the format()
on the previous line.
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's one character too long. I can wrap it or I can leave it? Which do you prefer?
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.
May be something like:
update_url = 'projects/{0}/models/{1}?updateMask=state.published'.format(
project_id, model_id)
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 with a suggestion.
tests/test_mlkit.py
Outdated
def _url(project_id, model_id): | ||
def _update_url(project_id, model_id): | ||
update_url = 'projects/{0}/models/{1}?updateMask=state.published' | ||
return BASE_URL + update_url.format(project_id, model_id) |
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.
May be something like:
update_url = 'projects/{0}/models/{1}?updateMask=state.published'.format(
project_id, model_id)
Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:
Discussion
If not, go file an issue about this before creating a pull request to discuss.
Testing
API Changes
us make Firebase APIs better, please propose your change in an issue so that we
can discuss it together.