Skip to content

Fix serialization of boolean attrib values#92

Open
hhussienn wants to merge 1 commit intoboostorg:developfrom
hhussienn:bool-serialize-patch
Open

Fix serialization of boolean attrib values#92
hhussienn wants to merge 1 commit intoboostorg:developfrom
hhussienn:bool-serialize-patch

Conversation

@hhussienn
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor

anadon commented Oct 15, 2018

@kinghussien Can you rebase your PR on devel?

@jzmaddock
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

Any thoughts on my comments above?

@anadon
Copy link
Copy Markdown
Contributor

anadon commented Nov 1, 2018

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

@anadon
Copy link
Copy Markdown
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
Copy Markdown
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