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/env headers #50

Merged

Conversation

GabrielBrandao1618
Copy link
Contributor

  • Add headers field to env
  • Add set header command
  • Add list headers command as well(still not configured though)

src/action/env.rs Fixed Show fixed Hide fixed
src/action/env.rs Fixed Show fixed Hide fixed
src/action/env.rs Fixed Show fixed Hide fixed
@GabrielBrandao1618 GabrielBrandao1618 marked this pull request as ready for review May 19, 2024 17:13
src/cli.rs Outdated
Comment on lines 240 to 245
#[derive(Debug, Subcommand)]
pub enum HeaderEnvCmd {
Set { headers: Vec<String> },
Ls,
Rm(action::env::HeaderRmArgs),
Get(action::env::HeaderGetArgs),

Choose a reason for hiding this comment

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

Could we use the same Cmd struct as for endpoint's Header? That would be useful to preserve documentation and the same usage.
The same is true for the Headers struct. Having 2 distinct implementations (endpoint::Headers and env::Headers) is kinda confusing, as they should both implement the same functionality.

This also avoids us needing to reimplementing all the traits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense!

src/action/ls.rs Outdated
@@ -156,6 +156,7 @@ pub fn cmd(ctx: &Ctx, args: Args) {
}

for child in node.children.iter() {
println!("This is a child: {}", child.value.handle());

Choose a reason for hiding this comment

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

Suggested change
println!("This is a child: {}", child.value.handle());

@EduardoRodriguesF
Copy link
Owner

In code, that is a great implementation now! However, I noticed a tiny bug that at one point was a problem with some other commands.
Listing headers will have double line breaks. Notice the difference in header ls to env header ls:

image

@GabrielBrandao1618, could you fix that before we merge?

@EduardoRodriguesF
Copy link
Owner

By the way, that probably happens because Headers struct's Display implementation already prints with line breaks. Try to compare yours with the header ls implementation just to make sure.

@GabrielBrandao1618
Copy link
Contributor Author

@EduardoRodriguesF done!

@EduardoRodriguesF EduardoRodriguesF merged commit a47ba05 into EduardoRodriguesF:master May 28, 2024
5 checks passed
@GabrielBrandao1618 GabrielBrandao1618 deleted the feat/env-headers branch June 27, 2024 12:01
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