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

add cli option --force-url-http for URL display #1314

Merged
merged 3 commits into from
Apr 28, 2024

Conversation

0spotter0
Copy link
Contributor

resolves #1313

  • Added cli flag --force-url-http
  • Added new formatting functions and tests.

@spenserblack
Copy link
Collaborator

I wonder if normalizing to HTTP should be the default behavior. 2 users can clone the exact same repo and get different stats based on their preferred clone strategy. I'm not sure if this is ideal. @o2sh thoughts?

src/cli.rs Outdated
Comment on lines 87 to 89
/// Always display the repo url as http
#[arg(long)]
pub force_url_http: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// Always display the repo url as http
#[arg(long)]
pub force_url_http: bool,
/// Display repository URL as HTTP
#[arg(long)]
pub http_url: bool,

src/info/url.rs Outdated
fn remove_token_from_url(url: &str) -> String {
let pattern = Regex::new(r"(https?://)([^@]+@)").unwrap();
let replaced_url = pattern.replace(url, "$1").to_string();
replaced_url
}

fn create_http_url(url: &str) -> String {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
fn create_http_url(url: &str) -> String {
fn create_http_url_from_ssh(url: &str) -> String {

src/info/url.rs Outdated
fn remove_token_from_url(url: &str) -> String {
let pattern = Regex::new(r"(https?://)([^@]+@)").unwrap();
let replaced_url = pattern.replace(url, "$1").to_string();
replaced_url
}

fn create_http_url(url: &str) -> String {
Copy link
Owner

Choose a reason for hiding this comment

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

maybe add a unit test specifically for this method

@o2sh
Copy link
Owner

o2sh commented Apr 25, 2024

I wonder if normalizing to HTTP should be the default behavior. 2 users can clone the exact same repo and get different stats based on their preferred clone strategy. I'm not sure if this is ideal. @o2sh thoughts?

I believe Onefetch should display the Git information verbatim unless asked otherwise. Knowing which protocol is used to sync with the remote can be important IMO.

* rename create_http_url to create_http_url_from_ssh
* add test for create_http_url_from_ssh
Copy link
Owner

@o2sh o2sh left a comment

Choose a reason for hiding this comment

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

Approved 🎉

@o2sh o2sh merged commit 5000452 into o2sh:main Apr 28, 2024
11 checks passed
@o2sh o2sh added the feat label Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CLI option to force URL format to http
3 participants