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

feat(#43): fakehub start, --port #60

Merged
merged 5 commits into from
Jul 11, 2024
Merged

feat(#43): fakehub start, --port #60

merged 5 commits into from
Jul 11, 2024

Conversation

h1alexbel
Copy link
Owner

@h1alexbel h1alexbel commented Jul 10, 2024

@l3r8yJ take a look, please

closes #43


PR-Codex overview

Focus: Refactoring CLI to support starting a server with logging.

Detailed summary

  • Added log::info for logging.
  • Implemented Server struct.
  • Updated Args struct with Command enum.
  • Added Start subcommand for starting the server.
  • Updated integration tests for help and start options.
  • Added integration test for starting the server.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@h1alexbel h1alexbel requested a review from l3r8yJ July 10, 2024 14:02
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 20.40%. Comparing base (57dfdf2) to head (6e32fbf).
Report is 11 commits behind head on master.

Files Patch % Lines
cli/src/main.rs 0.00% 11 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master      #60       +/-   ##
===========================================
- Coverage   58.82%   20.40%   -38.42%     
===========================================
  Files           5        5               
  Lines          17       49       +32     
===========================================
  Hits           10       10               
- Misses          7       39       +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@l3r8yJ l3r8yJ left a comment

Choose a reason for hiding this comment

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

@h1alexbel take a look at my comments below, please


#[test]
fn outputs_start_opts() -> Result<()> {
env!("CARGO_BIN_EXE_cli");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@h1alexbel btw I'm not sure that this is an required, can you try to remove it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

@l3r8yJ removed, test passed.

@h1alexbel h1alexbel requested a review from l3r8yJ July 11, 2024 06:33
@h1alexbel
Copy link
Owner Author

@l3r8yJ take a look, again please

Copy link
Collaborator

@l3r8yJ l3r8yJ left a comment

Choose a reason for hiding this comment

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

@h1alexbel take a look, please

#[test]
#[ignore]
fn starts_server() -> Result<()> {
env!("CARGO_BIN_EXE_cli");
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant env!

cli/src/args.rs Outdated Show resolved Hide resolved
cli/tests/integration_test.rs Outdated Show resolved Hide resolved
h1alexbel and others added 3 commits July 11, 2024 11:07
@h1alexbel
Copy link
Owner Author

@l3r8yJ updated

@h1alexbel h1alexbel requested a review from l3r8yJ July 11, 2024 08:12
Copy link
Collaborator

@l3r8yJ l3r8yJ left a comment

Choose a reason for hiding this comment

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

@h1alexbel looks good to me!

@h1alexbel h1alexbel merged commit dc1324f into master Jul 11, 2024
12 of 15 checks passed
@h1alexbel h1alexbel deleted the 43-start-port branch July 11, 2024 10:34
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.

args.rs:24-27: Handle port argument We should process the...
2 participants