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

see #199: add support for attr.name and attr.type #200

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ziodave
Copy link

@ziodave ziodave commented Feb 20, 2018

This is a proposal to add support for GraphML-Attributes in the GraphMLWriter.

@@ -13,6 +13,9 @@

import com.google.common.graph.EndpointPair;
import com.google.common.graph.Network;
import edu.uci.ics.jung.io.graphml.AttributeType;

import java.beans.XMLEncoder;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ziodave It looks to me that java.beans.XMLEncoder is never used, so this import should be removed if possible.

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Ta!


if (null != ds.attributeName)
bw.write(" attr.name=\"" + ds.attributeName.replace("\"", """) + "\"");
if (null != ds.attributeType) bw.write(" attr.type=\"" + ds.attributeType.getValue() + "\"");
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that these if statements are lacking braces. Please run mvn fmt:format to fix this and any other formatting errors I may have missed. :)

Copy link
Author

Choose a reason for hiding this comment

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

Uhm, the formatter isn't adding the braces, I'll add them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, very sorry, I thought it did! Thank your very much for adding them anyway. :)

@jbduncan
Copy link
Contributor

I'll defer to @jrtom now for evaluating the meat of this PR and the corresponding issue. :)

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