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

PR: Add remote filesystem API to the Remote client plugin #23381

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

hlouzada
Copy link
Contributor

@hlouzada hlouzada commented Dec 30, 2024

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)

Use a mixture of rest/websocket to comunicate with spyder-remote-services and provide a filesystem-like API to control remote file system.

This implements the client-side for spyder-ide/spyder-remote-services#11

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @hlouzada

subrepo:
  subdir:   "external-deps/spyder-kernels"
  merged:   "d7e4319b5"
upstream:
  origin:   "https://github.com/spyder-ide/spyder-kernels.git"
  branch:   "master"
  commit:   "d7e4319b5"
git-subrepo:
  version:  "0.4.9"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "cce3d93"
@pep8speaks
Copy link

pep8speaks commented Dec 30, 2024

Hello @hlouzada! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 115:80: E501 line too long (80 > 79 characters)

Line 2:80: E501 line too long (85 > 79 characters)

Line 28:80: E501 line too long (83 > 79 characters)
Line 36:80: E501 line too long (103 > 79 characters)
Line 394:80: E501 line too long (91 > 79 characters)
Line 427:65: E252 missing whitespace around parameter equals
Line 427:66: E252 missing whitespace around parameter equals

Line 49:80: E501 line too long (100 > 79 characters)
Line 70:80: E501 line too long (86 > 79 characters)
Line 134:80: E501 line too long (88 > 79 characters)
Line 136:80: E501 line too long (86 > 79 characters)
Line 152:80: E501 line too long (81 > 79 characters)

Line 114:80: E501 line too long (85 > 79 characters)

Comment last updated at 2025-01-23 21:32:18 UTC

@hlouzada hlouzada changed the title Add remote-files capability to remote-client plugin [WIP] PR: Add remote-files capability to remote-client plugin Dec 30, 2024
@hlouzada hlouzada marked this pull request as ready for review January 18, 2025 13:58
@hlouzada hlouzada changed the title [WIP] PR: Add remote-files capability to remote-client plugin PR: Add remote-files capability to remote-client plugin Jan 18, 2025
…xternal-deps/spyder-remote-services

subrepo:
  subdir:   "external-deps/spyder-remote-services"
  merged:   "e2f920d11"
upstream:
  origin:   "https://github.com/spyder-ide/spyder-remote-services"
  branch:   "feat/remote-file-service"
  commit:   "e2f920d11"
git-subrepo:
  version:  "0.4.9"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "cce3d93"
…xternal-deps/spyder-remote-services

subrepo:
  subdir:   "external-deps/spyder-remote-services"
  merged:   "83bfb48fd"
upstream:
  origin:   "https://github.com/spyder-ide/spyder-remote-services"
  branch:   "feat/remote-file-service"
  commit:   "83bfb48fd"
git-subrepo:
  version:  "0.4.9"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "cce3d93"
…xternal-deps/spyder-remote-services

subrepo:
  subdir:   "external-deps/spyder-remote-services"
  merged:   "b77bdbfb2"
upstream:
  origin:   "https://github.com/spyder-ide/spyder-remote-services"
  branch:   "feat/remote-file-service"
  commit:   "b77bdbfb2"
git-subrepo:
  version:  "0.4.9"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "cce3d93"
…xternal-deps/spyder-remote-services

subrepo:
  subdir:   "external-deps/spyder-remote-services"
  merged:   "4599cda25"
upstream:
  origin:   "https://github.com/spyder-ide/spyder-remote-services"
  branch:   "feat/remote-file-service"
  commit:   "4599cda25"
git-subrepo:
  version:  "0.4.9"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "cce3d93"
…xternal-deps/spyder-remote-services

subrepo:
  subdir:   "external-deps/spyder-remote-services"
  merged:   "bb7624869"
upstream:
  origin:   "https://github.com/spyder-ide/spyder-remote-services"
  branch:   "feat/remote-file-service"
  commit:   "bb7624869"
git-subrepo:
  version:  "0.4.9"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "cce3d93"
…/spyder-remote-services

subrepo:
  subdir:   "external-deps/spyder-remote-services"
  merged:   "2b8dcf2aa"
upstream:
  origin:   "https://github.com/spyder-ide/spyder-remote-services"
  branch:   "main"
  commit:   "2b8dcf2aa"
git-subrepo:
  version:  "0.4.9"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "cce3d93"
@ccordoba12 ccordoba12 added this to the v6.1.0 milestone Jan 22, 2025
@ccordoba12 ccordoba12 changed the title PR: Add remote-files capability to remote-client plugin PR: Add remote filesystem API to the Remote client plugin Jan 22, 2025
@hlouzada hlouzada force-pushed the add-remoteclient-file-service branch from 91ae29a to ab22af6 Compare January 23, 2025 13:40
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @hlouzada for your hard work to implement this functionality!

Comment on lines +86 to +90
early_return : bool, optional
Return the coroutine as a Future object before it is done
or wait for it to finish and return the result.
return_awaitable : bool, optional
Return the coroutine as an awaitable object instead of a Future.
Copy link
Member

@ccordoba12 ccordoba12 Jan 23, 2025

Choose a reason for hiding this comment

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

Please document these new kwargs in our Changelog (see previous API changes sections to get an idea about that).

Comment on lines +111 to +112
NotImplementedError: Can't instantiate abstract class
with abstract attributes.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
NotImplementedError: Can't instantiate abstract class
with abstract attributes.
NotImplementedError
When it's not possible to instantiate an abstract class with abstract
attributes.



class SpyderRemoteAPIManager:
"""Class to manage a remote server and its apis."""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Class to manage a remote server and its apis."""
"""Class to manage a remote server and its APIs."""

Comment on lines +104 to +105
GET_SERVER_INFO_COMMAND = (f"/${{HOME}}/.local/bin/micromamba run"
f" -n {SERVER_ENV} spyder-server info")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GET_SERVER_INFO_COMMAND = (f"/${{HOME}}/.local/bin/micromamba run"
f" -n {SERVER_ENV} spyder-server info")
GET_SERVER_INFO_COMMAND = (
f"/${{HOME}}/.local/bin/micromamba run"
f" -n {SERVER_ENV} spyder-server info"
)


@property
def server_url(self):
"""Return server URL
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Return server URL
"""
Return server URL

}

@AsyncDispatcher.dispatch(early_return=False)
async def test_list_dir(
Copy link
Member

Choose a reason for hiding this comment

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

This test and test_list_directories below are very similar. The latter seems more complete though, so why not remove this one?

async with file_api_class() as file_api:
assert await file_api.rmdir(self.remote_temp_dir) == {
"success": True
}
Copy link
Member

Choose a reason for hiding this comment

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

I think you should also check that the directory is not present anymore with file_api.ls for completeness.

Comment on lines +14 to +15
SCRIPT_URL = ("https://raw.githubusercontent.com/spyder-ide/"
f"{PACKAGE_NAME}/master/scripts")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SCRIPT_URL = ("https://raw.githubusercontent.com/spyder-ide/"
f"{PACKAGE_NAME}/master/scripts")
SCRIPT_URL = (
f"https://raw.githubusercontent.com/spyder-ide/"
f"{PACKAGE_NAME}/master/scripts"
)

This is an easier to read formatting.

Copy link
Member

Choose a reason for hiding this comment

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

This file doesn't contain any change related to your work, so you shouldn't have touched it.

Please edit the commit in which you introduced these changes and revert them. In other words, don't add another commit to do that (otherwise you'll break git blame for the rest of the team).

Copy link
Member

Choose a reason for hiding this comment

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

The same comment I left for spyder/plugins/remoteclient/widgets/connectiondialog.py applies to this one: please edit your git history to revert your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants