FileIO usage tweaks [DEVINFRA-640] #429

samvrlewis · 2022-02-16T04:50:45Z

Addresses some feedback from Stefan:

Improves the usage given in the help text
Shows a platform specific serial path in the help text, rather than always showing the Linux example
Shows an error on reading a non-existent file
Adds some context to file read/write errors
Improves the error messages for list and delete to not mention SRC
Adds error handling for given blank paths (eg: 192.168.222.0:)

The one bug I couldn't replicate was "Upload non existing file hangs" which does throw an error rather than hanging for me on Linux, so might be a Windows bug? Does seem a bit weird though given File::Open should return an error if the file doesn't exist. Will see if I can try on Windows and report back.

samvrlewis · 2022-02-16T04:52:06Z

console_backend/src/bin/files.rs


    /// The destination when transfering files
    dest: Option<Target>,

    /// List a directory
-    #[clap(long, short, conflicts_with_all = &["dest", "delete"])]
-    list: bool,
+    #[clap(long, short, value_name="TARGET", conflicts_with_all = &["dest", "delete"])]


Slight change, this will mean you won't be able to do swift-files 192.168.0.222:/data --list any longer but IMO makes reading the code slightly easier.

console_backend/src/bin/files.rs

samvrlewis · 2022-02-16T05:06:32Z

console_backend/src/fileio.rs

                }
            }
        }

        self.link.unregister(key);
-        Ok(())
+
+        if total_bytes_read == 0 {


I don't think it's possible to differentiate between whether the file attempting to be read is actually a 0 byte file or if the file read failed here (because the file didn't exist or because the user isn't allowed to read the file).

Thinking about this some more, maybe the error message should be more descriptive? Something like "File {} read was 0 bytes long. This could indicate that the file doesn't exist or is not accessible."

We should also potentially delete the file locally, as the user will see a 0 bytes file after running this at the moment.

What do others think?

I like the more informative message, IMO leaving the file is fine if we have the warning since the remote file could be a zero byte file.

silverjam · 2022-02-16T05:10:07Z

console_backend/src/bin/files.rs

-        .into_remote()
-        .context("--delete flag requires <SRC> to be a remote target")?;
+    let remote = target.into_remote().context(
+        "--delete flag requires <TARGET> to be a remote target of the form <HOST>:<PATH>",


Let's change to FILE_PATH here too

This could either mean the file actually is 0 bytes, the file doesn't exist, or the file isn't allowed to be accessed

samvrlewis · 2022-02-16T06:25:08Z

Tried uploading a non-existing file on Windows (10) and it seemed to fail in the same way Linux did. 🤷

(Which is a good thing, it should fail rather than hang)

Changed "Piksi" to "Swift device" and separated to the "cp to" and "cp from" examples

This gets clap-rs/clap#3421 which enables the help to show when no arguments are given and there are defaults.

samvrlewis · 2022-02-16T09:20:58Z

console_backend/Cargo.toml

@@ -31,7 +31,7 @@ serialport = { git = "https://github.com/swift-nav/serialport-rs.git", default-f
 directories = "4"
 anyhow = { version = "1", features = ["backtrace"] }
 serde_yaml = "0.8.23"
-clap = { version = "3.0.14", features = ["derive"] }
+clap = { git = "https://github.com/clap-rs/clap", rev="73847b32ff1245167a6d39ddd4fa90b93d1305ce", features = ["derive"] }


Upgraded to Clap on master in order to get this fix clap-rs/clap#3421 which was making the help not appear when no arguments were given (as all are now optional and some have defaults). It hasn't been included in a Clap release yet.

Fortuitously 3.1.0 was released overnight, so have upgraded to that now.

Sheesh just in the nick of time 😄

samvrlewis · 2022-02-16T09:25:53Z

Was also thinking that maybe we could take this opportunity before the tool is released to slightly rethink how the arguments work. Maybe a subcommand approach might work better?

Something like

swift-files transfer config.ini 192.168.222.0:/persistent/config.ini
swift-files transfer 192.168.222.0:/persistent/config.ini config.ini 
swift-files list 192.168.222.0:/persistent/
swift-files delete 192.168.222.0:/persistent/config.ini config.ini

Or maybe copy or cp instead of transfer? And/or ls instead of list and rm instead of delete? (Though I know a lot of Piksi people use windows users so maybe these are less appropriate?).

It feels slightly odd (to me) that list and delete are behind an option at the moment but copying a file isn't. But maybe others feel differently?

notoriaga · 2022-02-16T16:06:22Z

It feels slightly odd (to me) that list and delete are behind an option at the moment but copying a file isn't. But maybe others feel differently?

Yeah I was trying to make the tool more like scp and there was no clear analog there. I think subcommands for consistency make sense. I think you can alias subcommands so maybe

swift-files delete/rm
swift-files copy/cp
swift-files list/ls

probably would want to run that by stefan first

silverjam · 2022-02-16T18:05:01Z

It feels slightly odd (to me) that list and delete are behind an option at the moment but copying a file isn't. But maybe others feel differently?

Yeah I was trying to make the tool more like scp and there was no clear analog there. I think subcommands for consistency make sense. I think you can alias subcommands so maybe
swift-files delete/rm
swift-files copy/cp
swift-files list/ls
probably would want to run that by stefan first

If we do this we should probably mirror Linux, but alias with Windows equivalents. So rm/del, ls/dir, and cp/copy. The problem we had with subcommands last time was they weren't discoverable and as Steve mentioned, Stefan has already approved the current scheme so we'll need to get his sign off again.

samvrlewis · 2022-02-16T22:06:22Z

Ah I forgot about the subcommand discovery problem. I'll avoid rocking the boat then, seeing as Stefan is already happy and we need to deliver soon.

samvrlewis added 5 commits February 16, 2022 14:16

Improve error for empty remote path

ebd1574

Fix up serial in usage & some help strings

7278cb7

Add info to file errors

36ac5b3

Add error for reading non-existent file

bc4f81a

Improve usage and error messages

3503e6c

samvrlewis requested a review from a team February 16, 2022 04:50

samvrlewis commented Feb 16, 2022

View reviewed changes

silverjam reviewed Feb 16, 2022

View reviewed changes

console_backend/src/bin/files.rs Outdated Show resolved Hide resolved

samvrlewis commented Feb 16, 2022

View reviewed changes

PATH -> FILE_PATH for delete

c66d5eb

silverjam reviewed Feb 16, 2022

View reviewed changes

samvrlewis added 2 commits February 16, 2022 16:13

Fixed missed PATH -> FILE_PATH

6ea9add

Make error on 0 byte file read more informative

02baf5e

This could either mean the file actually is 0 bytes, the file doesn't exist, or the file isn't allowed to be accessed

silverjam requested a review from notoriaga February 16, 2022 06:17

Merge branch 'main' into slewis/more-fileio-feedback

813c82b

samvrlewis and others added 2 commits February 16, 2022 17:36

Update usage

d6212dd

Changed "Piksi" to "Swift device" and separated to the "cp to" and "cp from" examples

Use master version of Clap

bf62569

This gets clap-rs/clap#3421 which enables the help to show when no arguments are given and there are defaults.

samvrlewis commented Feb 16, 2022

View reviewed changes

Fix formatting of cargo.toml

c4df77b

Upgrade to Clap 3.1.0

5212be1

silverjam approved these changes Feb 16, 2022

View reviewed changes

notoriaga approved these changes Feb 16, 2022

View reviewed changes

Merge branch 'main' into slewis/more-fileio-feedback

135c172

samvrlewis enabled auto-merge (squash) February 16, 2022 23:18

samvrlewis merged commit ae40d71 into main Feb 16, 2022

samvrlewis deleted the slewis/more-fileio-feedback branch February 16, 2022 23:40

silverjam mentioned this pull request Mar 22, 2022

fix typo in settings GGL -> GLL [CPP-700] #475

Merged

silverjam mentioned this pull request Apr 20, 2022

ci: bump cargo-make to 0.35.10, pin taplo-cli at 0.6.0 #525

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FileIO usage tweaks [DEVINFRA-640] #429

FileIO usage tweaks [DEVINFRA-640] #429

samvrlewis commented Feb 16, 2022

samvrlewis Feb 16, 2022

samvrlewis Feb 16, 2022

silverjam Feb 16, 2022

silverjam Feb 16, 2022

samvrlewis commented Feb 16, 2022 •

edited

samvrlewis Feb 16, 2022

samvrlewis Feb 16, 2022

john-michaelburke Feb 16, 2022

samvrlewis commented Feb 16, 2022 •

edited

notoriaga commented Feb 16, 2022 •

edited

silverjam commented Feb 16, 2022

samvrlewis commented Feb 16, 2022

FileIO usage tweaks [DEVINFRA-640] #429

FileIO usage tweaks [DEVINFRA-640] #429

Conversation

samvrlewis commented Feb 16, 2022

samvrlewis Feb 16, 2022

Choose a reason for hiding this comment

samvrlewis Feb 16, 2022

Choose a reason for hiding this comment

silverjam Feb 16, 2022

Choose a reason for hiding this comment

silverjam Feb 16, 2022

Choose a reason for hiding this comment

samvrlewis commented Feb 16, 2022 • edited

samvrlewis Feb 16, 2022

Choose a reason for hiding this comment

samvrlewis Feb 16, 2022

Choose a reason for hiding this comment

john-michaelburke Feb 16, 2022

Choose a reason for hiding this comment

samvrlewis commented Feb 16, 2022 • edited

notoriaga commented Feb 16, 2022 • edited

silverjam commented Feb 16, 2022

samvrlewis commented Feb 16, 2022

samvrlewis commented Feb 16, 2022 •

edited

samvrlewis commented Feb 16, 2022 •

edited

notoriaga commented Feb 16, 2022 •

edited