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

[main < T load, parse] Implement xml.load and xml.parse #329

Merged
merged 30 commits into from Sep 7, 2023

Conversation

mpintaric55334
Copy link
Contributor

Description

Please briefly explain the changes you made here.

Pull request type

  • Bugfix
  • Algorithm/Module
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Related issues

Delete if this PR doesn't resolve any issues. Link the issue if it does.

######################################

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

######################################

Copy link
Collaborator

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

This looks solid and does the job, just make sure to do this:

  • change the tests to return something with a guaranteed order, not maps whose items’ ordering is not guaranteed
  • format Python code

python/xml_module.py Outdated Show resolved Hide resolved
python/xml_module.py Outdated Show resolved Hide resolved
python/xml_module.py Outdated Show resolved Hide resolved
python/xml_module.py Outdated Show resolved Hide resolved
python/xml_module.py Outdated Show resolved Hide resolved
python/xml_module.py Outdated Show resolved Hide resolved
@antepusic
Copy link
Collaborator

antepusic commented Aug 28, 2023

@antoniofilipovic The tests use example XMLs from a third-party website. Thinking about reliability, I think it’s better to make our own example XMLs and host them on some Memgraph repo.

@antoniofilipovic antoniofilipovic added status: change PR reviewed - needs changes and removed status: ready PR is ready for review labels Aug 29, 2023
@mpintaric55334
Copy link
Contributor Author

@antepusic @antoniofilipovic shouldnt the map order always be same in tests?

@mpintaric55334 mpintaric55334 added status: ready PR is ready for review and removed status: change PR reviewed - needs changes labels Aug 29, 2023
Copy link
Collaborator

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

About map order: The map contains elements from children, whose value is taken from the ElementTree parsed by .fromstring.

result[children_name] = [
    parse_element(child, simple) for child in children
]

After checking, I think their ordering is consistent so the tests are fine in that respect.

python/xml_module.py Show resolved Hide resolved
@antepusic antepusic added status: discuss it PR commented - needs discussion status: change PR reviewed - needs changes and removed status: ready PR is ready for review labels Aug 30, 2023
@antoniofilipovic
Copy link
Collaborator

mple XMLs from a third-party website. Thinking about reliability, I think it’s better to make our own example XMLs and host them on some Memgraph repo.

@antepusic I wouldn't do that for now. If it starts failing, we can check then.

@antoniofilipovic
Copy link
Collaborator

About map order: The map contains elements from children, whose value is taken from the ElementTree parsed by .fromstring.

result[children_name] = [
    parse_element(child, simple) for child in children
]

After checking, I think their ordering is consistent so the tests are fine in that respect.

@antepusic so is PR ready to be merged or not?

@antepusic
Copy link
Collaborator

It’s ready then! 🚀

@antepusic antepusic added status: ship it PR approved type: module and removed status: change PR reviewed - needs changes status: discuss it PR commented - needs discussion labels Aug 30, 2023
@antoniofilipovic antoniofilipovic merged commit 6000bbb into main Sep 7, 2023
4 checks passed
@antoniofilipovic antoniofilipovic deleted the T-add-xml-load-and-parse branch September 7, 2023 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants