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

chore: output address of uploaded file #733

Merged
merged 4 commits into from Sep 21, 2023

Conversation

jacderida
Copy link
Contributor

@jacderida jacderida commented Sep 15, 2023

  • b8487f4 chore: output address of uploaded file

    Other minor adjustments for more compact text output.

  • b5666ce feat: provide a files ls command

    Gives the user to list the files they have uploaded, along with their addresses. We were storing
    this data anyway, but it's serialized to binary, so it's not human readable. A little interface on
    the client could be useful for users.

  • bdfd4f7 chore: clarify files download usage

    The documentation for the arguments for the command are clarified, and we introduce a failure if
    only one of the name/address pair is used, as both are necessary.

    Also fix up some code from a complicated rebase.

  • ba4809e chore: store uploaded files list as text

    For the uploaded files list, rather than serialize to binary, we use a simple text format. The user
    can now see the addresses of all the files they've uploaded, and use this information to retrieve
    them, if need be. We are also now just using one file, appending to it for each upload.

    Now that this file is human readable, the files ls command was deemed unnecessary and was
    therefore removed.

    With respect to the files commands, I've also taken the opportunity to make the language in the
    text output more concise and matter of fact. For example, we don't really need to use things like
    exclamation marks in the text.

Description

Summary generated by Reviewpad on 15 Sep 23 21:01 UTC

This pull request includes the following changes:

  • In cli.rs:

    • Added a clarification comment for the timeout field.
    • Added a clarification comment for the concurrency field.
    • Added a clarification comment for the no_verify field.
  • In main.rs:

    • Added a new FilesCmds variant, Ls, which lists all files uploaded by the current user.
    • Added handling for the Ls variant in the files_cmds function.
  • In files.rs:

    • Added a new FilesCmds variant, Ls, which lists all files uploaded by the current user.
    • Added a new function, offline_files_cmds, which handles the Ls variant when offline.
    • Added handling for the Ls variant in the files_cmds function.
    • Added a new function, list_files, which lists all files uploaded by the current user.
    • Various changes, including clarifying comments and error handling improvements.
  • In file_apis.rs:

    • Clarified print statements.
  • In wallet.rs:

    • Adjusted print statement for completed transfers.

@reviewpad reviewpad bot requested a review from joshuef September 15, 2023 21:01
@reviewpad reviewpad bot added Medium Medium sized PR waiting-for-review labels Sep 15, 2023
@joshuef
Copy link
Contributor

joshuef commented Sep 18, 2023

I actually think instead of 050103b we should just make that file human readable.

files ls I could see having the implication it may work on any platform, but its just what you have done on this comp... (something that we will likely have later)... Feels like a plaster around just removing the bincode encoding on that file to me.

Copy link
Contributor

@joshuef joshuef left a comment

Choose a reason for hiding this comment

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

can we remove 050103b

I'd rather we made the file human readable and that was how you can check. Doesn't feel like it needs to be in the cli to me.

@jacderida
Copy link
Contributor Author

can we remove 050103b

I'd rather we made the file human readable and that was how you can check. Doesn't feel like it needs to be in the cli to me.

OK cool. Serialize to JSON?

@joshuef
Copy link
Contributor

joshuef commented Sep 19, 2023

OK cool. Serialize to JSON?

Just a space separated list perhaps? (newline per file?)

We could do json, but not sure it's worth it... treat yourself as the consumer of this (you are!) so whatever seems best!

@reviewpad reviewpad bot added Small Pull request is small and removed Medium Medium sized PR labels Sep 19, 2023
@reviewpad reviewpad bot added Medium Medium sized PR and removed Small Pull request is small labels Sep 20, 2023
@jacderida jacderida force-pushed the file-upload-address branch 2 times, most recently from 4d04ce3 to ba4809e Compare September 20, 2023 19:51
Other minor adjustments for more compact text output.
Gives the user to list the files they have uploaded, along with their addresses. We were storing
this data anyway, but it's serialized to binary, so it's not human readable. A little interface on
the client could be useful for users.
The documentation for the arguments for the command are clarified, and we introduce a failure if
only one of the name/address pair is used, as both are necessary.

Also fix up some code from a complicated rebase.
For the uploaded files list, rather than serialize to binary, we use a simple text format. The user
can now see the addresses of all the files they've uploaded, and use this information to retrieve
them, if need be. We are also now just using one file, appending to it for each upload.

Now that this file is human readable, the `files ls` command was deemed unnecessary and was
therefore removed.

With respect to the `files` commands, I've also taken the opportunity to make the language in the
text output more concise and matter of fact. For example, we don't really need to use things like
exclamation marks in the text.
@joshuef joshuef added this pull request to the merge queue Sep 21, 2023
Merged via the queue into maidsafe:main with commit 3c1432c Sep 21, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Medium Medium sized PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants