Skip to content
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

Refactor naming and expand client docstrings #71

Merged
merged 7 commits into from
May 10, 2019

Conversation

willingc
Copy link
Member

@willingc willingc commented Mar 23, 2019

  • Adds/edits docstrings for clients
  • update filenames to avoid confusion
    - reorder classes in notebook client file for maintainability
  • add autodoc doc pages

@todo
Copy link

todo bot commented Mar 23, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 4830650 in #71. cc @willingc.

@todo
Copy link

todo bot commented Mar 23, 2019

verify that *args is not needed

# TODO: verify that *args is not needed
self.kernel_id = kernel_id
self.name = name
self.last_activity = last_activity
self.execution_state = execution_state
self.connections = connections


This comment was generated by todo based on a TODO comment in 4830650 in #71. cc @willingc.

@todo
Copy link

todo bot commented Mar 23, 2019

double check *args, **kwargs are not necessary

# TODO: double check *args, **kwargs are not necessary
self.path = path
self.name = name
self.type = session_type
self.kernel = KernelInfo(**kernel)
self.notebook = notebook


This comment was generated by todo based on a TODO comment in 4830650 in #71. cc @willingc.

@todo
Copy link

todo bot commented Mar 23, 2019

refactor from lambda to a def

# TODO: refactor from lambda to a def
nb_client_gen = lambda: (NotebookClient(x) for x in list_running_servers())
sessions = {x.url: x.sessions for x in nb_client_gen()}
@classmethod
def current_server(cls):


This comment was generated by todo based on a TODO comment in 4830650 in #71. cc @willingc.

@willingc willingc requested a review from mpacer March 23, 2019 23:46
@todo
Copy link

todo bot commented Mar 23, 2019

Add a check for success

# TODO: Add a check for success
resp = requests.put(target_url, **req)


This comment was generated by todo based on a TODO comment in 4830650 in #71. cc @willingc.

@willingc
Copy link
Member Author

@mpacer let's keep iterating on the clients

@todo
Copy link

todo bot commented Mar 23, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 755969c in #71. cc @willingc.

@todo
Copy link

todo bot commented Mar 24, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 697987e in #71. cc @willingc.

@willingc willingc changed the title Refactor naming and expand client docstrings [WIP] Refactor naming and expand client docstrings Mar 24, 2019
@todo
Copy link

todo bot commented Mar 25, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in b96f684 in #71. cc @willingc.

Copy link
Member

@mpacer mpacer left a comment

Choose a reason for hiding this comment

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

So much greatness here! Thank you @willingc.

There were a couple of things that I would have wanted changed around the object APIs, but this is such a dramatic improvement that I'd be willing to merge this even though I think those changes need to be made in a later PR.

bookstore/client/nb_client.py Outdated Show resolved Hide resolved
bookstore/client/nb_client.py Outdated Show resolved Hide resolved
bookstore/client/nb_client.py Outdated Show resolved Hide resolved
bookstore/client/nb_client.py Show resolved Hide resolved
bookstore/client/nb_client.py Outdated Show resolved Hide resolved
bookstore/client/nb_client.py Outdated Show resolved Hide resolved
bookstore/client/nb_client.py Outdated Show resolved Hide resolved
bookstore/client/nb_client.py Outdated Show resolved Hide resolved
bookstore/client/nb_client.py Outdated Show resolved Hide resolved
bookstore/client/nb_client.py Show resolved Hide resolved
@todo
Copy link

todo bot commented May 9, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in ca51029 in #71. cc @willingc.

@todo
Copy link

todo bot commented May 9, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 3fb5b93 in #71. cc @willingc.

@todo
Copy link

todo bot commented May 9, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 763dd0b in #71. cc @willingc.

@todo
Copy link

todo bot commented May 9, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 77e1476 in #71. cc @willingc.

@todo
Copy link

todo bot commented May 9, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 06e2ba5 in #71. cc @willingc.

@todo
Copy link

todo bot commented May 9, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 56d8216 in #71. cc @willingc.

@todo
Copy link

todo bot commented May 9, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in e9b3330 in #71. cc @willingc.

@todo
Copy link

todo bot commented May 9, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 29ddf01 in #71. cc @willingc.

@todo
Copy link

todo bot commented May 9, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 5a0d843 in #71. cc @willingc.

@willingc willingc changed the title [WIP] Refactor naming and expand client docstrings Refactor naming and expand client docstrings May 9, 2019
@willingc
Copy link
Member Author

willingc commented May 9, 2019

@mpacer @MSeal I've gone back and simplified this PR to docstring changes, a missed import, autodoc of files, and black autoformat. Let's go ahead and merge. Then we can begin tackling the other PR. We should decide whether to do a phone call review or break it up into smaller PRs. Thanks.

Copy link
Member

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

LGTM, I'll let @mpacer merge as I'm only getting up to speed on bookstore code / opinions.

@todo
Copy link

todo bot commented May 10, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in 8addca3 in #71. cc @willingc.

@todo
Copy link

todo bot commented May 10, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in a4c1392 in #71. cc @willingc.

@todo
Copy link

todo bot commented May 10, 2019

(Clarify) We want to test our bookstore endpoints, but it's no fun having

TODO: (Clarify) We want to test our bookstore endpoints, but it's no fun having
to do this in an insecure fashion. Better would be to have some security in
place.
Example
-------


This comment was generated by todo based on a TODO comment in a0a78e6 in #71. cc @willingc.

@willingc willingc merged commit 9564865 into nteract:master May 10, 2019
@willingc willingc deleted the clarify-naming branch May 10, 2019 17:31
@mpacer mpacer added this to the 2.3 milestone Jun 28, 2019
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.

3 participants