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

Added function element_encode() #32

Merged
merged 4 commits into from
May 6, 2022

Conversation

lykciv
Copy link
Contributor

@lykciv lykciv commented Apr 29, 2022

Q A
Type feature
BC Break no
Fixed issues #31

Summary

@veewee
Copy link
Owner

veewee commented Apr 30, 2022

Looks awesome, thanks!
Added a few minor nitpicks to make it even better 😲

public function test_it_encodes_to_element(string $xml, array $data)
{
$actual = element_encode($data);
static::assertXmlStringEqualsXmlString($xml, $actual);
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe you can make sure the XML header is not available in the result here?

docs/encoding.md Outdated
@@ -32,6 +32,7 @@ The encoding components consist out of following functions

- [document_encode](#document_encode): Encodes an array into a DOM [Document](dom.md).
- [element_decode](#element_decode): Decodes a DOMElement into an array.
- [element_encode](#element_encode): Encodes an array into an XML string.
Copy link
Owner

Choose a reason for hiding this comment

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

This has the same comment as xml_encode, might be confusing for the developers

docs/encoding.md Outdated
@@ -93,6 +94,30 @@ In the example above, we run an XSD validator before parsing the XML into an arr

More information about [the PHP format can be found here](#php-format).

#### element_encode

Encode transforms an array into an XML string.
Copy link
Owner

Choose a reason for hiding this comment

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

Same here: looks like a better idea to differentiate and link between the two functions at the top.

@lykciv
Copy link
Contributor Author

lykciv commented May 6, 2022

Thank you for the feedback. I have made some changes accordingly. I hope the documentation is now a bit clearer as well.

@veewee veewee merged commit 19daf66 into veewee:master May 6, 2022
@veewee
Copy link
Owner

veewee commented May 6, 2022

Yes it is. Thanks again for your PR!

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