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

Enable check-status by default #155

Merged
merged 9 commits into from
Jun 12, 2021
Merged

Enable check-status by default #155

merged 9 commits into from
Jun 12, 2021

Conversation

ducaale
Copy link
Owner

@ducaale ducaale commented Jun 11, 2021

  • Introduce strict compatibility mode
  • Enable --check-status by default if strict compatibility mode is not on.

Resolves #144

src/cli.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Now that the name matters, it might be nicer to replace clap::Arg::with_name(flag) by clap::Arg::with_name(&flag[2..]). That way the arguments will be named check-status and no-check-status, nicely symmetrical.

Can you implement a strict compatibility mode?

README.md Outdated Show resolved Hide resolved
src/cli.rs Outdated
Comment on lines 398 to 400
if matches!(bin_name, Some("http") | Some("https")) {
cli.strict_compat_mode = true;
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe I should also add an env for enabling strict_compat_mode? This could be helpful for testing purposes and for users who have installed xh via a package manager.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Update: I have now added XH_HTTPIE_COMPAT_MODE env variable.

@ducaale ducaale requested a review from blyxxyz June 12, 2021 13:19
Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Looks good! Option<bool> is better than my idea about two fields 🙂

README.md Outdated Show resolved Hide resolved
Co-authored-by: Jan Verbeek <jan.verbeek@posteo.nl>
README.md Outdated Show resolved Hide resolved
Co-authored-by: Jan Verbeek <jan.verbeek@posteo.nl>
@ducaale ducaale merged commit d0ee7b2 into develop Jun 12, 2021
@ducaale ducaale deleted the check-status-by-default branch June 12, 2021 14:15
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.

2 participants