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 XML support #79

Closed
wants to merge 1 commit into from
Closed

add XML support #79

wants to merge 1 commit into from

Conversation

tmm1
Copy link
Contributor

@tmm1 tmm1 commented May 12, 2023

cc #78

@earthboundkid
Copy link
Owner

I am pretty happy with this, but I think in 2023 XML is sort of niche, so it doesn't need to be a pair of methods on Builder, and it could just be functions instead. Builder already has so many methods that it junks up the docs. Also I'm not sure about it, but I think if it's a method then you have to import encoding/xml along with this package, but if it's not maybe it gets tree shaken out? Might want to experiment with it to figure that out.

Also, it's nice to have an example test. Is there a good public API that uses XML to demo?

@tmm1
Copy link
Contributor Author

tmm1 commented May 12, 2023

Hmm, I am definitely using the builder methods. Having to set application/xml each time manually would be a pain without BodyXML.

@tmm1
Copy link
Contributor Author

tmm1 commented May 12, 2023

Also, it's nice to have an example test. Is there a good public API that uses XML to demo?

I looked for an xmlplaceholder api but didn't see one pop up right away.

@earthboundkid
Copy link
Owner

earthboundkid commented May 12, 2023

Having to set application/xml each time manually would be a pain without BodyXML.

Something I wonder about for BodyJSON is if people are inconvenienced when they want to set a different content type than application/json. For XML, I bet there are servers that require the content type to be application/xml+myapp.v3 or whatever. Maybe there should be some way to signal what the default content type for a BodyGetter is while still letting you override it. Not sure how it would work though.

@earthboundkid
Copy link
Owner

earthboundkid commented May 12, 2023

I did a quick test of putting a function that imports encoding/xml next to another that doesn't and then not using the xml function, and it ends up not being tree shaken out. So, if we add this, everyone's build will end up including encoding/xml even if they don't use it. Probably not a big deal in practice, but some people want smaller binaries.

What if we move these into a subfolder, maybe reqxml.Body(contentType string, obj any) requests.Config and reqxml.To(any) requests.Handler? Body can default to using application/xml as the content type when it's an empty string.

Use would look like

err := requests.
  URL("http://whatever.com").
  Config(reqxml.Body("", req)).
  Handler(reqxml.To(&res)).
  Fetch(ctx)

@earthboundkid
Copy link
Owner

Here is a funny XML example:

func ExampleBuilder_ToXML() {
	type CD struct {
		Title   string  `xml:"TITLE"`
		Artist  string  `xml:"ARTIST"`
		Country string  `xml:"COUNTRY"`
		Price   float64 `xml:"PRICE"`
		Year    int     `xml:"YEAR"`
	}
	type Catalog struct {
		CDs []CD `xml:"CD"`
	}
	var cat Catalog
	err := requests.
		URL("https://www.w3schools.com/xml/cd_catalog.xml").
		ToXML(&cat).
		Fetch(context.Background())
	if err != nil {
		fmt.Println("Error!", err)
	}
	for _, cd := range cat.CDs {
		if cd.Price > 10 && cd.Year < 1990 {
			fmt.Printf("%s - %s $%.2f", cd.Artist, cd.Title, cd.Price)
		}
	}
	// Output:
	// Bob Dylan - Empire Burlesque $10.90
}

@earthboundkid
Copy link
Owner

I rolled your commit into #82. Let me know if it looks good to you.

@earthboundkid
Copy link
Owner

Merged in #82.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants