Skip to content

Implement file client functions #119

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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

chemicstry
Copy link

@chemicstry chemicstry commented Apr 30, 2025

This PR implements service client functions for uavcan.file.* services.

I grouped everything under file-client command group, but since there was no other example of command group, I'm not sure if I implemented it properly. I would also like to get feedback about the progress/error reporting as it was a bit confusing how it's supposed to work.

Posting this as a draft, because of TODO:

  • List/GetInfo
  • List/GetInfo pretty printing
  • Modify (remove)
  • Modify (copy/move)
  • Modify (touch)
  • Read
  • Write
  • Docs

@pavel-kirienko
Copy link
Member

pavel-kirienko commented May 12, 2025

EDIT: please wait for this to be merged: #120

@pavel-kirienko
Copy link
Member

Okay please merge the main branch into yours and you should get a working CI again. Sorry for the turmoil, keeping this up takes some effort.

@chemicstry
Copy link
Author

Thank for fixing the CI, maintaning such projects is hard.

I just noticed that pycyphal has a FileClient2 application, which I could have used instead of reimplementing everything, dooh. I was about to scrap everything and rewrite, but it is missing progress reporting. Should I add optional progress callbacks to pycyphal, or just leave this lower-level implementation as is?

@pavel-kirienko
Copy link
Member

Progress callbacks in FileClient2 would be a welcome addition!

@chemicstry
Copy link
Author

I think this should be ready, just needs the pycyphal PR to be merged. Also probably a release?

The ls function has proper json and yaml output, but I'm not sure how to do it in "human readable" format. Couldn't find any examples in the repo and the whole formatting infrastructure looks too alien to me as a non-python person.

@@ -0,0 +1,168 @@
# Copyright (c) 2021 OpenCyphal
# This software is distributed under the terms of the MIT License.
# Author: Pavel Kirienko <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Please update the authorship remark. I didn't write this module.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, ok, I thought file headers are consistent for entire repository 🤔

Copy link
Member

Choose a reason for hiding this comment

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

no

@@ -22,6 +22,6 @@ def _read_package_file(name: str) -> str:
__copyright__ = f"Copyright (c) 2020 {__author__} <{__email__}>"
__license__ = "MIT"

from .main import main as main, subcommand as subcommand, Purser as Purser, pass_purser as pass_purser
from .main import main as main, subcommand as subcommand, commandgroup as commandgroup, Purser as Purser, pass_purser as pass_purser
Copy link
Member

Choose a reason for hiding this comment

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

this line is too long

@@ -0,0 +1,443 @@
# Copyright (c) 2021 OpenCyphal
# This software is distributed under the terms of the MIT License.
# Author: Pavel Kirienko <[email protected]>
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
# Author: Pavel Kirienko <[email protected]>

from pycyphal.application.file import FileClient2

src = PurePosixPath(src)
dst = Path(dst) if dst else Path(src.name)
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 the default behavior should be to stream the data into stdout in binary mode (sys.stdout.buffer.write(...)), because this is the unix way and is probably expected by default.

Copy link
Author

Choose a reason for hiding this comment

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

I would argue that cp src dst is a more unix way than cat src > dst for moving files. Also, when using pipes the progress can not be displayed, though correct me on this.

Comment on lines +395 to +396
@click.argument("src")
@click.argument("dst", required=False)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would be better to require dst and make src optional, defaulting to stdin in binary mode instead? This would mirror the read command.

is_flag=True,
help="""
Ignore nodes that fail to respond to the first RPC-service request instead of reporting an error
assuming that the register service is not supported.
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
assuming that the register service is not supported.
assuming that the file service is not supported.

@pavel-kirienko
Copy link
Member

PyCyphal v1.23.0 should go live in about 40 min

@chemicstry
Copy link
Author

Sorry for stalling on this, I got different priorities at work ATM. I will get back to this PR when I find time

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.

2 participants