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

Draft: feat: Support Sample Responses #22

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tomharmon
Copy link

  • WIP

See issue #16 for background and motivation

Solution

  • Will fill this section in when MR is no longer a draft.
  • TL;DR for the current draft is that this adds a PaneFocus::SampleResponse variant and a Vec<SampleResponse> to a request.

@tomharmon tomharmon mentioned this pull request Aug 17, 2024
@tomharmon tomharmon force-pushed the feat/sample-responses branch from cf01b42 to f3eea61 Compare August 17, 2024 21:18
@tomharmon tomharmon force-pushed the feat/sample-responses branch from f3eea61 to f6a4297 Compare August 17, 2024 22:13
@wllfaria
Copy link
Owner

I'll be adding some comments here mainly to just discuss about things, feel free to point out anything you want aswell.

@@ -60,16 +61,18 @@ impl PaneFocus {
PaneFocus::Sidebar => PaneFocus::ReqUri,
PaneFocus::ReqUri => PaneFocus::Editor,
PaneFocus::Editor => PaneFocus::Preview,
PaneFocus::Preview => PaneFocus::Sidebar,
PaneFocus::Preview => PaneFocus::SampleResponse,
PaneFocus::SampleResponse => PaneFocus::Sidebar,
Copy link
Owner

Choose a reason for hiding this comment

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

personally, I think Sample responses may be best suited to live within Pretty/Raw response viewers, i'm not sure how we should allow the user to switch between actual current response and saved sample responses, but I think a whole new pane for this is not really needed, what you think?

#[serde(skip_serializing_if = "Option::is_none")]
pub body: Option<String>,
#[serde(skip_serializing_if = "IndexMap::is_empty")]
pub headers: IndexMap<String, Vec<String>>,
Copy link
Owner

Choose a reason for hiding this comment

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

I assume here the idea behind the IndexMap is to preserve order of headers on serialization? I asked this question with the headers for requests, I'm still not sure if this is a huge deal, but I guess its fine

@wllfaria wllfaria force-pushed the main branch 2 times, most recently from 07bacb0 to 92413dd Compare October 2, 2024 13:17
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