Skip to content

test: add AbstractGraph interface tests and CI fixes (#42 split)#43

Open
mahmudsudo wants to merge 2 commits intoJuliaGraphs:masterfrom
mahmudsudo:pr-interface-tests
Open

test: add AbstractGraph interface tests and CI fixes (#42 split)#43
mahmudsudo wants to merge 2 commits intoJuliaGraphs:masterfrom
mahmudsudo:pr-interface-tests

Conversation

@mahmudsudo
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Member

@Krastanov Krastanov 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 close to done, thanks for taking it on. I left a few comments on things that seem to be incorrect or potentially problematic.

It seems @nenadilic84 has started doing something related to the interface tests in PR #35 and has a bit more thorough interface tests.

Could you either remove your interface tests so I can also merge #35 or cherry-pick the tests and bug fix? Or is this something you are already trying to address in #44 (I think not, it seems fairly different).

Maybe the easiest way forward to incorporate all the improvements you guys are submitting is to merge this as only CI fixes, then merge a rebased #35, then proceed with rebasing and reviewing your #44 -- let me know what your preferences are.

This is of course assuming @nenadilic84 has the time to fix up #35

Given @nenadilic84 was first to start working on this bounty, I want to make sure he has a chance to clean up his PR as well. It is a bit messy when multiple folks start working on the same bounty, but the whole point is to provide an onboarding experience for more junior contributors, so the mess is worth it. I do suspect you guys would end up splitting the bounty given that you are working on various sides of the same issue.

@mahmudsudo
Copy link
Copy Markdown
Author

This looks close to done, thanks for taking it on. I left a few comments on things that seem to be incorrect or potentially problematic.

It seems @nenadilic84 has started doing something related to the interface tests in PR #35 and has a bit more thorough interface tests.

Could you either remove your interface tests so I can also merge #35 or cherry-pick the tests and bug fix? Or is this something you are already trying to address in #44 (I think not, it seems fairly different).

Maybe the easiest way forward to incorporate all the improvements you guys are submitting is to merge this as only CI fixes, then merge a rebased #35, then proceed with rebasing and reviewing your #44 -- let me know what your preferences are.

This is of course assuming @nenadilic84 has the time to fix up #35

Given @nenadilic84 was first to start working on this bounty, I want to make sure he has a chance to clean up his PR as well. It is a bit messy when multiple folks start working on the same bounty, but the whole point is to provide an onboarding experience for more junior contributors, so the mess is worth it. I do suspect you guys would end up splitting the bounty given that you are working on various sides of the same issue.

i cherrypicked the interface tests and implemented other corrections and pushed to #45

@mahmudsudo mahmudsudo requested a review from Krastanov March 30, 2026 14:01
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