Skip to content

Conversation

@ind1xa
Copy link
Contributor

@ind1xa ind1xa commented Aug 24, 2023

Description

Implemented import_util.graphml and export_util.graphml

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):

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

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

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

@ind1xa ind1xa added the status: ready PR is ready for review label Aug 24, 2023
@ind1xa ind1xa self-assigned this Aug 24, 2023
Comment on lines 146 to 155
if graphML:
properties = {
key: convert_to_isoformat_graphML(vertex.properties.get(key))
for key in vertex.properties.keys()
}
else:
properties = {
key: convert_to_isoformat(vertex.properties.get(key))
for key in vertex.properties.keys()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if graphML:
properties = {
key: convert_to_isoformat_graphML(vertex.properties.get(key))
for key in vertex.properties.keys()
}
else:
properties = {
key: convert_to_isoformat(vertex.properties.get(key))
for key in vertex.properties.keys()
}
if graphML:
properties = {
key: convert_to_isoformat_graphML(vertex.properties.get(key))
for key in vertex.properties.keys()
}
continue
properties = {
key: convert_to_isoformat(vertex.properties.get(key))
for key in vertex.properties.keys()
}

Copy link
Contributor Author

@ind1xa ind1xa Aug 29, 2023

Choose a reason for hiding this comment

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

but wont continue go to the next vertex in ctx.graph.vertices and lines below will be skipped?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes but isn't that okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really, i want those properties to be added to the node in both cases, just in different format (depending if its for graphml or not)

Copy link
Contributor

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

Good work, left few comments to do

Copy link
Contributor

@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.

Left a few comments, I'll take a more detailed look later too :)

@antoniofilipovic antoniofilipovic added status: change PR reviewed - needs changes and removed status: ready PR is ready for review labels Aug 29, 2023
@ind1xa ind1xa changed the base branch from main to E-add-create-functions August 30, 2023 05:30
@ind1xa ind1xa changed the base branch from E-add-create-functions to main August 30, 2023 05:31
@ind1xa ind1xa removed the status: change PR reviewed - needs changes label Aug 30, 2023
@antepusic antepusic added the status: change PR reviewed - needs changes label Aug 30, 2023
@ind1xa ind1xa added status: ready PR is ready for review and removed status: change PR reviewed - needs changes labels Sep 1, 2023
Copy link
Contributor

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

Looks good to me

@antoniofilipovic antoniofilipovic added status: ship it PR approved and removed status: ready PR is ready for review labels Sep 5, 2023
@antoniofilipovic antoniofilipovic merged commit 9faa147 into main Sep 8, 2023
@antoniofilipovic antoniofilipovic deleted the T350-MAGE-implement-graphml branch September 8, 2023 12:59
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.

4 participants