deprecate names! #1986
deprecate names! #1986
Conversation
| @@ -38,6 +38,36 @@ const ≇ = !isequal | |||
| end | |||
| end | |||
|
|
|||
| @testset "additional rename! tests" begin | |||
| Random.seed!(123) | |||
| for i in 1:1000 | |||
There was a problem hiding this comment.
Why do we need this brute-force approach? Can't you just choose a set of values that cover all possible cases?
There was a problem hiding this comment.
It takes 0.1 second to run these 1000 tests. I add this because I was not sure if we cover all possible cases manually (the code for rename! in #1974 is tricky, so I prefer to be on a safe side).
There was a problem hiding this comment.
Hmm, but are you sure that with 1000 iterations we cover all cases? :-)
Anyway, please add a comment explaining a bit why we do this.
There was a problem hiding this comment.
Thank you for the docs fixes - a keen eye as always 😄.
I have added a note in the test. In general I am convinced that 1000 should cover all cases (I have chosen column count - 8 and names set so that with high probability all cases are covered). But I am not sure 😢, if I were sure I would not write a randomized test.
In general we want to be sure that cyclical renames like :X=>:Y, :Y=>:Z, :Z=>:X for various cycle lengths are processed correctly and also that non cyclical renames like :X=>:Y, :Z=>:X are processed correctly also when they happen in combination and in the presence of other columns that are not renamed and in two subcategories: a) the proposed renaming scheme is valid and b) the proposed renaming scheme is invalid (so as you see there are really many possible combinations of cases and it is really hard to make sure that you cover all of them if you do not generate the tests randomly; for sure the existing tests have not covered all the possibilities).
Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>
Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>
|
CI fails due to coverage. Otherwise this should be good for a final review. Thank you. |
Fixes #1943.
@nalimilan after this is the last PR (apart from some already open ones) that I think is needed before 0.20 release.