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

Igreen1 #896

Merged
merged 19 commits into from
Nov 13, 2020
Merged

Igreen1 #896

merged 19 commits into from
Nov 13, 2020

Conversation

igreen1
Copy link
Collaborator

@igreen1 igreen1 commented Nov 13, 2020

Addresses issue #886 by finally fixing the tests that didn't allow the last PR to function properly. The fix adds an expression and a meta property to all import networks so later attempts to access these properties returns the expected undefined instead of an error.

Addresses issue #444 by increasing the margin of the self-referential edges

The fix for #541 was removed from my branch due to a sudden issue with the library. I suspect it is an issue with my local machine and hope to resolve it tonight (after my lab report)

libxmljs was added as a validator, then put into the current parser as a validator.
However, I am having issues testing this
…these properties when the data does not contain them
Unfortunately, sif is very confusing so I wasn't able to do this there
However, by keeping the optionals in the update-app, it doesn't metter

So, now GraphML will work and SIF will work
Better fix for .sif files
…, not arrays to match data format

Fixed tests to include new graphml and sif imports
The previous commit has a failed build because of this error
Minimize delta between commits :)
Changed arc offset back to 20, 30 was too much
Changed self-referential offset calculations to use offset variable instead of magic numbers. Still magic numbers in calculation as offset is too small for x calculations, but I think this solution is still better
Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

In terms of addressing the issue, this looks good to me, although the number of times that meta/expression had to be manually added makes me wonder whether some consolidation is called for here (e.g., some kind of default “initialize empty network” function or some such.

Perhaps a refactor to do next.

@dondi dondi merged commit 116d24a into beta Nov 13, 2020
dondi added a commit that referenced this pull request Nov 13, 2020
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