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

Use WeakKeyIdDicts.jl #1666

Closed
haberdashPI opened this issue Apr 12, 2024 · 4 comments
Closed

Use WeakKeyIdDicts.jl #1666

haberdashPI opened this issue Apr 12, 2024 · 4 comments

Comments

@haberdashPI
Copy link

haberdashPI commented Apr 12, 2024

Hey there! I copy/pasted the internal WeakKeyIdDict implementation found in this repo to make use of it in other packages (rather than depending on an implementation internal to this package; and really, I don't need to depend on all of the stuff in here just to use a WeakKeyIdDict 😄 ).

You can find it here: https://github.com/beacon-biosignals/WeakKeyIdDicts.jl

I'm wondering if it would make sense to have AbstractAlgebra depend on it rather than use the internal implementation.

@fingolfin
Copy link
Member

Hey there, I am glad that code is useful to you, and I am happy if you want to put it into a package.

However, I'd appreciate if this did a bit better job at documenting the license, copyrigh and origin of this code. And so should we, really!

Regarding the license file in your repository, I find it rather inappropriate that it starts with "Copyright (c) 2021 Beacon Biosignals" -- that's just wrong in every possibly way.

You go on to quote the AA license. Unfortunately (and you can't know that, of course) that license file is also in a bad state, and for this particular bit of code, the copyright line "Copyright (c) 2014-2016: William Hart, Tommy Hofmann, Claus Fieker, Fredrik Johansson" is also completely inappropriate: none of these people worked on that code in question, and certainly not in that time range ;-)

Let's unravel this a bit then. The code evolved through these PRs on various repositories:

  1. WIP/RFC: Converted WeakKeyDict to hash & compare with object-id JuliaLang/julia#28161
  2. WeakKeyIdDict implementation (keeping WeakKeyDict) JuliaLang/julia#28182
  3. Implemented WeakKeyIdDict JuliaCollections/DataStructures.jl#402
  4. Added data-structure WeakKeyIdDict oscar-system/Oscar.jl#1872
  5. Add WeakKeyIdDict #1419 (which was finally merged)

The first four of these were all by @mauro3 who definitely should get credit. I then took the code in the penultimate PR and turned it into the final PR. I then spent some time bug hunting and rewrote multiple parts of it to match latest Julia WeakKeyDict implementation (this was necessary to fix a bunch of bugs!), and also added several more tests to get the coverage up

The Julia codebase is under MIT licenses, and I think @mauro3 also had that license on his code, and I also don't mind; so it would be fine to have just the MIT license in your repository. But I think it would be reasonable to briefly explain the genesis of the code at the bottom of your README.md and/or in the license file. As to copyright, perhaps this for the license file.

Copyright (c) 2016-2023 The Julia Team
Copyright (c) 2018-2023 Mauro Werder
Copyright (c) 2023 Max Horn
Copyright (c) 2024 Beacon Biosignals

@fingolfin
Copy link
Member

As to changing AA to depend on your package: I don't see a benefit for us in doing so at this point.

@haberdashPI
Copy link
Author

Thanks for spelling that out @fingolfin, I'll update the license.

@haberdashPI
Copy link
Author

haberdashPI added a commit to beacon-biosignals/WeakKeyIdDicts.jl that referenced this issue Apr 15, 2024
As per Nemocas/AbstractAlgebra.jl#1666

---------

Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
@thofma thofma closed this as not planned Won't fix, can't repro, duplicate, stale Apr 15, 2024
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

No branches or pull requests

3 participants