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 to_xml and from_xml filter plugin #56

Merged
merged 33 commits into from
Apr 9, 2021

Conversation

ashwini-mhatre
Copy link
Contributor

@ashwini-mhatre ashwini-mhatre commented Mar 25, 2021

SUMMARY
ISSUE TYPE
  • Feature Pull Request
  • Docs Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

@ashwini-mhatre ashwini-mhatre changed the title WIP: Add xml_to_json and json_to_xml filter plugin Add xml_to_json and json_to_xml filter plugin Mar 26, 2021
Copy link
Member

@ganeshrn ganeshrn left a comment

Choose a reason for hiding this comment

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

Also please add unit and integration tests

plugins/filter/json_to_xml.py Outdated Show resolved Hide resolved
plugins/filter/json_to_xml.py Outdated Show resolved Hide resolved
plugins/filter/json_to_xml.py Outdated Show resolved Hide resolved
plugins/filter/json_to_xml.py Outdated Show resolved Hide resolved
plugins/module_utils/common/json_to_xml.py Outdated Show resolved Hide resolved
plugins/module_utils/common/xml_to_json.py Outdated Show resolved Hide resolved
plugins/module_utils/common/xml_to_json.py Outdated Show resolved Hide resolved
plugins/module_utils/common/xml_to_json.py Outdated Show resolved Hide resolved
plugins/module_utils/common/xml_to_json.py Outdated Show resolved Hide resolved
plugins/module_utils/common/xml_to_json.py Outdated Show resolved Hide resolved
@cidrblock
Copy link
Collaborator

putting this out for conversation, does simply to_xml and from_xml make better sense as a name, since from the user perspective the actual json representation of the data is never seen since it is immediately picked up by ansible and made available as native data structure

@ashwini-mhatre
Copy link
Contributor Author

As its only specific to json and xml conversion and xml to json i think xml_to_json and json_to_xml are valid names. if we are supporting from any data format to xml then to_xml sounds good. @cidrblock @ganeshrn please let me know your inputs on this.

@cidrblock
Copy link
Collaborator

I was thinking more about this pattern:

{{ some_variable | to_json }}
{{ some_variable | to_yaml }}

where the transform is being done from data ansible has (which is a python object, not specifically json)

in the case of from_xml, the user is left with a native data object as well.

WDYT ganesh?

@ganeshrn
Copy link
Member

ganeshrn commented Mar 29, 2021

@cidrblock to_json \ from_yaml and from_json \ from_yaml is a conversion between json/yaml object <--> to
it's string representation but in this case we are changing the structure of data itself so IMO from_xml name might be misleading as it might refer we are returning an xml object. How about xml_to_native and native_to_xml

@ashwini-mhatre ashwini-mhatre added this to the April 2021 milestone Mar 29, 2021
@ashwini-mhatre
Copy link
Contributor Author

Also please add unit and integration tests

added uts and integration testcases

@ganeshrn
Copy link
Member

@cidrblock @ashwini-mhatre As per your earlier comment we can rename the plugins to to_xml and from_xml with proper documentation to highlight to_xml converts native python dictionary to xmlstring and from_xml converts xml string to native python dictionary

@ashwini-mhatre
Copy link
Contributor Author

@cidrblock @ashwini-mhatre As per your earlier comment we can rename the plugins to to_xml and from_xml with proper documentation to highlight to_xml converts native python dictionary to xmlstring and from_xml converts xml string to native python dictionary

@ganeshrn @cidrblock will rename these plugins as to_xml and from_xml.

tttlease enter the commit message for your changes. Lines starting
t Please enter the commit message for your changes. Lines starting
@ashwini-mhatre ashwini-mhatre changed the title Add xml_to_json and json_to_xml filter plugin Add to_xml and from_xml filter plugin Mar 31, 2021
@ashwini-mhatre
Copy link
Contributor Author

recheck

README.md Outdated Show resolved Hide resolved
plugins/filter/from_xml.py Outdated Show resolved Hide resolved
plugins/filter/from_xml.py Outdated Show resolved Hide resolved
plugins/filter/from_xml.py Outdated Show resolved Hide resolved
plugins/filter/from_xml.py Outdated Show resolved Hide resolved
plugins/filter/to_xml.py Outdated Show resolved Hide resolved
plugins/filter/to_xml.py Outdated Show resolved Hide resolved
plugins/filter/to_xml.py Outdated Show resolved Hide resolved
plugins/filter/to_xml.py Outdated Show resolved Hide resolved
plugins/filter/to_xml.py Outdated Show resolved Hide resolved
@ashwini-mhatre
Copy link
Contributor Author

recheck

@ashwini-mhatre ashwini-mhatre added mergeit Merge a pull request and removed mergeit Merge a pull request labels Apr 8, 2021
@ansible-zuul ansible-zuul bot merged commit 43e7754 into ansible-collections:main Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergeit Merge a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants