Generics migration #231
Generics migration #231
Conversation
This attempts to migrate this library in the least invasive way by preserving as much of the original API as possible. It does not change the tests in a meaningful way nor does it attempt to upgrade any logic that can be simplified or improved with generics. This is purely an API migration, and still requires a lot of additional work to be fully ready.
|
Impressive effort! |
|
Thanks! I'm still working on it, and I can happily break it up into multiple PRs to make this easier to review but the bulk of it is there :) Any initial thoughts? |
|
The changes are a bit tricky to isolate to separate PRs, so I believe it's easier for both of us doing it in bulk - we can count on the tests in the end that things work as expected. |
|
On another note, do you have suggestions on how to solve for versioning, so we don't break it for people using the old non-generic version? This is something that concerned me in the past and I have not found an elegant solution for it. |
|
I do yeah. There was a conversation about this in #179 and I think the move is simply to mint a V2 version of this codebase, instead of trying to maintain multiple versions of these data structures in different directories. If some sort of critical fix is needed for the non-generic versions of these, a v1 version can always be minted again |
|
Kudos, SonarCloud Quality Gate passed!
|
|
@PapaCharlie much appreciation for your effort. So regarding the versioning to do something like this: #179 (comment) I've added you as a collaborator here so you should have access to work directly on the repo. |
|
Yeah basically we can kick the non-generic implementation to a v1 branch and keep it around if we need to fix it, but the default branch should be the v2/generics branch |
|
Btw, like I mentioned in the PR description, this change doesn't actually change the API. As a follow up for I guess a v3 we should align with this proposal: golang/go#61405 and consolidate all the implementations of |
|
Btw, thanks for making a collaborator! Was your intent to let me check this in as-is and let me work on it further? Or did you still intend to review it? |
|
Let me also take a look over the weekend at this one more time to create the branches v1 and v2, and v2 would be pushed to master, right? Great job indeed, but just give me until Sunday to review and then merge if that is ok. |
|
Please, take your time! I just wasn't sure what your plan was. As for the strategy for v1/v2, we can actually just check this in as-is after I update the
Let me know if I can help with that at all! I think having a migration guide is also in order? I can write up that part |
|
waiting for this merge. |
|
Hey @emirpasic, have you had a chance to take a look at this? |
|
Oops!! My mistake, accidentally merged this... |
|
I reverted the merge, reopening the PR |
|
No I think it is fine to merge @PapaCharlie . I have been away for a few month due to personal things... I believe this is good enough, so feel free to merge. |
|
Alright! Sounds good. No worries, hope everything's alright. I will be working on further changes (especially to testing, which is very repeated) to this in the near future. In the meantime, I'll release this as v2.0.0-alpha |
This attempts to migrate this library in the least invasive way by preserving as much of the original API as possible. It does not change the tests in a meaningful way nor does it attempt to upgrade any logic that can be simplified or improved with generics. This is purely an API migration, and still requires a lot of additional work to be fully ready.