[#98] Clean up entrypoint CLI #366

orecham · 2024-09-03T14:20:00Z

Notes for Reviewer

Refactoring of commands.rs:

add abstraction to make logic testable
remove duplicate code
utilize anyhow since errors only require printing, no unique error handling
fix bug where args were not forwarded to subcommands
remove need for --dev flag
- dev builds will instead have their build type appended (e.g. foo-debug or foo-release)
utilize cargo_metadata to more reliably determine the target dir to search for dev builds

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Unit tests have been written for new behavior
Public API is documented
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Relates #98

codecov · 2024-09-03T14:57:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.48%. Comparing base (b4b8d9c) to head (e83a259).
Report is 8 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #366   +/-   ##
=======================================
  Coverage   79.48%   79.48%           
=======================================
  Files         193      193           
  Lines       22716    22716           
=======================================
+ Hits        18055    18056    +1     
+ Misses       4661     4660    -1

see 4 files with indirect coverage changes

elBoberido

Looks good in general. Just a few remarks.

iceoryx2-cli/iox2/src/commands.rs

elfenpiff · 2024-09-04T00:49:21Z

iceoryx2-cli/iox2/src/commands.rs

-    if let Some(arguments) = args {
-        command.args(arguments);
+    #[test]
+    fn test_paths() {


You could actually let the test work in the real environment. Take a look at iceoryx2-bb/posix/tests/file_tests.rs. Here, I utilize the iceoryx2_bb_posix::config::test_directory() call, which provides a platform-independent location to a directory where you can create files and directories for testing.

This would allow you to get rid of the traits and the generic parameters.

What do you think?

orecham · 2024-09-11T07:42:30Z

iceoryx2-cli/iox2/tests/commands_tests.rs

+    use std::io::Write;
+
+    #[cfg(windows)]
+    const MINIMAL_EXE: &[u8] = include_bytes!("minimal.exe");


@elfenpiff @elBoberido This is the easiest way I could test the execution on windows. Thoughts?

I am fine with it - I would rename it to no-op.exe since it is a bit more descriptive but this is not a must.

orecham · 2024-09-11T07:43:24Z

iceoryx2-bb/testing/src/assert.rs

@@ -173,6 +173,34 @@ macro_rules! assert_that {
            }
        }
    };
+    ($lhs:expr, contains_match |$element:ident| $predicate:expr) => {


@elfenpiff To use assert_that I had to extend it with this. Thoughts?

I like it. But a look at this file reminds me that we have to start to document this at some point ...

orecham · 2024-09-11T07:44:13Z

iceoryx2-cli/iox2/Cargo.toml

-thiserror = { workspace = true }
+clap = { workspace = true }
+human-panic = { workspace = true }
+cargo_metadata = { version = "0.18.1" }


Should this and tempfile be moved to the workspace Cargo.toml? I left them here since I doubt they will be used elsewhere.

Yes. The idea is that the whole workspace has unified and compatible dependencies. Therefore, everything has to be in the workspace and is then used with workspace = true by the individual crates of that workspace.

iceoryx2-cli/iox2/src/lib.rs

elfenpiff · 2024-09-11T08:11:25Z

iceoryx2-bb/testing/src/assert.rs

@@ -173,6 +173,34 @@ macro_rules! assert_that {
            }
        }
    };
+    ($lhs:expr, contains_match |$element:ident| $predicate:expr) => {


I like it. But a look at this file reminds me that we have to start to document this at some point ...

elfenpiff · 2024-09-11T08:13:00Z

iceoryx2-cli/iox2/Cargo.toml

-thiserror = { workspace = true }
+clap = { workspace = true }
+human-panic = { workspace = true }
+cargo_metadata = { version = "0.18.1" }


Yes. The idea is that the whole workspace has unified and compatible dependencies. Therefore, everything has to be in the workspace and is then used with workspace = true by the individual crates of that workspace.

elfenpiff · 2024-09-11T08:22:04Z

iceoryx2-cli/iox2/tests/commands_tests.rs

+    use std::io::Write;
+
+    #[cfg(windows)]
+    const MINIMAL_EXE: &[u8] = include_bytes!("minimal.exe");


I am fine with it - I would rename it to no-op.exe since it is a bit more descriptive but this is not a must.

iceoryx2-cli/iox2/tests/commands_tests.rs

elBoberido

Looks good. Just one question regarding a potential ambiguous constant name.

iceoryx2-cli/iox2/src/commands.rs

… types

orecham force-pushed the iox2-98-cleanup-iox-entrypoint-cli branch from f082455 to a503b85 Compare September 3, 2024 14:39

orecham force-pushed the iox2-98-cleanup-iox-entrypoint-cli branch from a503b85 to 8536d97 Compare September 3, 2024 15:01

elBoberido reviewed Sep 3, 2024

View reviewed changes

iceoryx2-cli/iox2/src/commands.rs Show resolved Hide resolved

iceoryx2-cli/iox2/src/commands.rs Outdated Show resolved Hide resolved

iceoryx2-cli/iox2/src/commands.rs Outdated Show resolved Hide resolved

elfenpiff changed the title ~~Clean up entrypoint CLI~~ [#98] Clean up entrypoint CLI Sep 4, 2024

elfenpiff reviewed Sep 4, 2024

View reviewed changes

orecham force-pushed the iox2-98-cleanup-iox-entrypoint-cli branch 9 times, most recently from e3b1b25 to 67f46a2 Compare September 11, 2024 07:40

orecham commented Sep 11, 2024

View reviewed changes

iceoryx2-cli/iox2/src/lib.rs Show resolved Hide resolved

orecham force-pushed the iox2-98-cleanup-iox-entrypoint-cli branch 5 times, most recently from 9e719f8 to ff0ce66 Compare September 11, 2024 08:04

elfenpiff reviewed Sep 11, 2024

View reviewed changes

orecham force-pushed the iox2-98-cleanup-iox-entrypoint-cli branch 2 times, most recently from d895d19 to c6cb7d4 Compare September 11, 2024 20:28

orecham added 3 commits September 12, 2024 11:15

[eclipse-iceoryx#98] Use anyhow throughout CLI implementation

f2b8d8e

[eclipse-iceoryx#98] Pass empty optional when no command args provided

0dc4340

[eclipse-iceoryx#98] Ensure arguments are forwarded onto sub-commands

eed1a2f

orecham force-pushed the iox2-98-cleanup-iox-entrypoint-cli branch 2 times, most recently from 3e254d8 to 89355a0 Compare September 12, 2024 09:28

elBoberido reviewed Sep 12, 2024

View reviewed changes

iceoryx2-cli/iox2/src/commands.rs Outdated Show resolved Hide resolved

orecham added 2 commits September 12, 2024 15:01

[eclipse-iceoryx#98] Test the CLI

653af95

[eclipse-iceoryx#98] Use unique names for different development build…

e83a259

… types

orecham force-pushed the iox2-98-cleanup-iox-entrypoint-cli branch from 89355a0 to e83a259 Compare September 12, 2024 13:01

elBoberido approved these changes Sep 12, 2024

View reviewed changes

orecham merged commit 2d1f01d into eclipse-iceoryx:main Sep 12, 2024
54 of 55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#98] Clean up entrypoint CLI #366

[#98] Clean up entrypoint CLI #366

orecham commented Sep 3, 2024 •

edited by elBoberido

Loading

codecov bot commented Sep 3, 2024 •

edited

Loading

elBoberido left a comment

elfenpiff Sep 4, 2024

orecham Sep 11, 2024

elfenpiff Sep 11, 2024

orecham Sep 11, 2024

elfenpiff Sep 11, 2024

orecham Sep 11, 2024

elfenpiff Sep 11, 2024

elfenpiff Sep 11, 2024

elfenpiff Sep 11, 2024

elfenpiff Sep 11, 2024

elBoberido left a comment

[#98] Clean up entrypoint CLI #366

[#98] Clean up entrypoint CLI #366

Conversation

orecham commented Sep 3, 2024 • edited by elBoberido Loading

Notes for Reviewer

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

codecov bot commented Sep 3, 2024 • edited Loading

Codecov Report

elBoberido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

orecham commented Sep 3, 2024 •

edited by elBoberido

Loading

codecov bot commented Sep 3, 2024 •

edited

Loading