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

Fixes Self-loop label bug when using compaction #1081

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

Eddykasp
Copy link
Contributor

Self-loop labels were previously omitted during compaction. This PR moves them together with everything else.

Self-loop labels were previously omitted during compaction. This PR
moves them together with everything else.
@Eddykasp Eddykasp added this to the Release 0.9.2 milestone Oct 21, 2024
@Eddykasp Eddykasp added the alg-layered Affects the ELK Layered algorithm. label Oct 21, 2024
Copy link
Contributor

@soerendomroes soerendomroes left a comment

Choose a reason for hiding this comment

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

This looks good. Do you think that a test would be a good idea for compaction?

@Eddykasp
Copy link
Contributor Author

This looks good. Do you think that a test would be a good idea for compaction?

I looked into the current tests for compaction and they are only algorithmic tests on the CGraph i.e. not on the underlying graph elements. It would probably be good to add such a test to check that compaction has been properly applied to all elements of the graph. This would help avoid bugs such as this in the future, but I'm not sure what the best approach for that would be. Maybe we could merge this fix now, and open an issue to add better compaction tests in the future.

@Eddykasp Eddykasp merged commit 099fd0d into eclipse-elk:master Nov 6, 2024
7 checks passed
@Eddykasp
Copy link
Contributor Author

fixes #953

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alg-layered Affects the ELK Layered algorithm.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants