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

Fix serialization of boolean attrib values #92

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix serialization of boolean attrib values #92

wants to merge 1 commit into from

Conversation

hhussienn
Copy link

Fix serialization of boolean attribute values from '1' or '0' to 'true' or 'false' per this ticket: https://svn.boost.org/trac/boost/ticket/11092

Fix serialization of boolean attribute values from '1' or '0' to 'true' or 'false' per this ticket: https://svn.boost.org/trac/boost/ticket/11092
@hhussienn hhussienn changed the title Patch graphml.hpp Fix serialization of boolean attrib values Jun 8, 2017
@anadon
Copy link
Contributor

anadon commented Aug 31, 2018

I'm helping out with the PR backlog. Looks like you have a code change, no test, and no new examples will be needed. Is this needed, or more semantics? I'm wary to have things change unless there is a need. That isn't to say that this is bad, but I need to be careful with this kind of PR. This is to let you know and help me prioritize PR's.

@anadon
Copy link
Contributor

anadon commented Oct 15, 2018

@kinghussien Can you rebase your PR on devel?

@jzmaddock
Copy link
Contributor

I'm going to close and re-open to trigger a CI build.

Question - while this looks sensible (and reasonably innocuous to me) - do we have a way to test this? Based on a quick look, I couldn't see anything in the GraphML spec that requires boolean attributes to be either boolalpha or numeric. I would hope that most software would support both.... anyone have any experience of this?

@jzmaddock jzmaddock closed this Oct 16, 2018
@jzmaddock jzmaddock reopened this Oct 16, 2018
@anadon
Copy link
Contributor

anadon commented Nov 1, 2018

@jzmaddock gcc6 is timing out a lot. Should it be disabled? I may fork this, rebase on devel, and make a PR in order to close this out.

@jzmaddock
Copy link
Contributor

I've pushed a fix for the timeout to develop.

Any thoughts on my comments above?

@anadon
Copy link
Contributor

anadon commented Nov 1, 2018

I don't have experience. Might the graphviz people have any experience?

@anadon
Copy link
Contributor

anadon commented Nov 5, 2018

@jzmaddock Looking into this further, I think NetworkX not working is significant enough to pull in the change. If this does break code, it will be very easy to fix, but surely adds further integration and support with tools in the area. I'll vote to merge it in.

@anadon
Copy link
Contributor

anadon commented Nov 6, 2018

@jzmaddock Could you trigger the failed test? It was a time out.

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.

3 participants