-
Notifications
You must be signed in to change notification settings - Fork 39
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
Remove sort(::Dict)
piracy (as bugfix)
#110
Conversation
Let us try it: @nanosoldier |
I've deployed some fixes. @nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
I’m assuming the timeouts are unrelated, but the two failures are both real. Interestingly the OPFSampler one hits a Compat.jl method with this PR due to JuliaLang/Compat.jl#804 I’m not sure DataStructures should count bc we can just remove those tests, but probably we should test all of its dependents as it reexports the OrderedCollections api |
Lots of timeouts though... I've added some debugging code to see what's going on. Hopefully you don't mind me using this PR to debug PkgEval; feel free to remove those comments afterwards 🙂 @nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
Looks ok to me, the failure in https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/f07867e_vs_c3687a8/OPFSampler.primary.log is not a failure in the package itself but a failure that only occurs from the testing code (which has a trivial fix). |
Shall we do DataStructures dependents too, as it reexports OrderedCollections api? I guess I’d need to pass them all in a long list to nanosoldier |
Let us try DataStructures deps: @nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
I am a pkg-eval-reader-noob, but I only see 1 failure, which looks like it would be fixed by AdamAker/BasicAkerRelationalScore.jl#3. So this looks pretty good to me. If there is consensus, we could proceed by:
Perhaps folks can 👍 or 👎 this post to chime in that they agree it is OK to remove the deprecated sort piracy as a bugfix release, given these results. And in a few days if there is agreement we can proceed. |
I wouldn't bother. |
Back to debugging PkgEval (ignore this comment): @nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. |
Looks like we have agreement that this is the right way to proceed, so I will merge & tag tomorrow if no objections arise |
sort(::Dict)
piracy (as bugfix)
So i have continued thinking. I think we should wait either for a major release of OrderedCollections.jl I don't think this is e.g. causing a huge amount of invalidations etc. So I do not see that there is a rush. It also came up for unrelated releated reasons in a Triage call with @LilithHafner. @JeffBezanson thinks this type piracy is good XD. |
Ok, thanks very much for the feedback! I was actually motivated by Base adding (then reverting) sorting for iterables, which seemed to make this piracy worse to me, since the type you would get would depend on you dependency stack (and whether or not you had Revise loaded, which likely means a difference between the devs machine and prod). On the other hand, having noisier deprecations first makes a lot of sense too. |
Horrifying. |
Fixes #25
Alternative to #109 - if PkgEval says none of our direct dependents are relying on this deprecated pathway, perhaps we can remove the sort deprecation as a bugfix. (Suggested by @KristofferC).
@maleadt would it be possible to enable nanosoldier on this repo? (per https://github.com/JuliaCI/Nanosoldier.jl#reverse-ci-for-packages).