Fixes for HomogeneousList #6773
Conversation
|
Hi there @MSeifert04 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here |
CHANGES.rst
Outdated
| ``HomogeneousList``. [#6773] | ||
|
|
||
| - Make ``HomogeneousList`` work with iterators and generators when creating the | ||
| instance, extending it or using when setting a slice. [#6773] |
There was a problem hiding this comment.
It is also used in test_logger.py although that test is marked as xfail.
I am nervous to port this back into 2.x. If things works in votable now, why risk unintended breakage (however small the risk might be). Why not just do your super thing here and set milestone to 3.0?
|
I changed the milestone to 3.0. But does it make sense to "fix" it at all given that current usage didn't suffer from the bugs and nobody else is using it (as far as we know). |
|
@MSeifert04 - Fixing it in 3.0 makes sense to me even if the current usage doesn't run into any bugs. It fits nicely into the series of your other refactorings. |
There was a problem hiding this comment.
I think it is good that we clean this up. If we put this in v3 and we get VO Table users complaining that this broke their code, then we can at least ask them to fall back to v2 until a fix can be made.
Let me take this for a spin with my Cone Search cronjob. If nothing blows up, I'll approve this next week.
|
|
||
| def test_homogeneous_list_setitem_works_with_slice(): | ||
| l = collections.HomogeneousList(int, [1, 2, 3]) | ||
| l[0:1] = [10, 20, 30] |
There was a problem hiding this comment.
Shouldn't the slice be [:] or [0:3]? Why is it set to [0:1]? (You can tell that I never used it, LoL)
There was a problem hiding this comment.
The [0:1] replaces the first element with the sequence, [0:3] would replace the first, second and third element with the sequence and [:] would replace the whole sequence.
In this case the result is [10, 20, 30, 2, 3], for [0:3] it would be [10, 20, 30] and for [:] it would also be [10, 20, 30]. I have no preference which one should be tested, I just picked one (the one which I though of first) because all defers to lists method and it's just important to check that it gets there. :)
There was a problem hiding this comment.
Hmm, okay, that is fine. I was wondering why none of the tests check for the resultant values.
There was a problem hiding this comment.
oh, I can add result checks.
There was a problem hiding this comment.
Why not? It doesn't take much more time to do the checks and makes the intent clear. Thanks!
|
|
||
|
|
||
| def test_homogeneous_list_works_with_generators(): | ||
| hl = collections.HomogeneousList(int, (i for i in range(3))) |
There was a problem hiding this comment.
Does it not take generators as-is (without the list comprehension part)?
There was a problem hiding this comment.
It does, but I changed it to use extend internally (which didn't work with generators), this was just to make sure it works.
Sounds good to me :) Can the cron job pick up not-merged-PRs? 😄 |
Yes. 😄 |
|
Looks all good to me (didn't know either we had this...) |
I don't know how user-facing this class is but it's used in votable. However I noticed several very weird (and wrong) stuff in there.
Instead of using the hard-coded
list.methodnamecalls we can usesuper()in astropy 3.x but that wouldn't work for 2.0.x. If this is merged one could consider a follow-up PR that usessuperthere.