-
Notifications
You must be signed in to change notification settings - Fork 160
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
Fix PositionProperty with from < 1 #2056
Fix PositionProperty with from < 1 #2056
Conversation
I think you mean |
This is something that should definitely be mentioned in release notes, as it could easily affect the behavior of some existing code (although I'd hope anybody who actually used it would have noticed the bug, too, and report it; since we saw no report, hopefully just nobody used this feature?) |
if from < 1 then | ||
from:= 1; | ||
if from < 0 then | ||
from:= 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exact same bug is in the code 30 lines before, please fix it there, too.
For cream with a cherry on top, perhaps also add a test case? :-) |
7fe6c63
to
2666f5a
Compare
Done, with cream and a cherry on top. |
2666f5a
to
e11fba9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, awesome :-)
We can now also think about whether this should go into GAP 4.9.x? Or just 4.10?
e11fba9
to
b2d63c5
Compare
Thanks @fingolfin I don't really have an opinion about whether it should go in 4.9.x or 4.10.x. Probably earlier is better since it's a bug fix, but maybe there are some reasons not to include it in 4.9.x? |
While this is technically a bug fix, there is a slight chance that some code relies on the existing broken behaviour. As such, putting this into GAP 4.9 would be a small risk. Personally, I'd be OK with that, but it's a decision that should be made consciously. In order to put this into stable-4.9, though, this PR should be retargeted and then updated (so that it based on that branch, not |
I feel this should be in 4.9, I suspect it is more likely someone has written incorrect code without realising, than they are relying on the behaviour. |
I changed the base to stable-4.9, but now @james-d-mitchell needs to rebase |
b2d63c5
to
739359c
Compare
This PR fixes a bug in
PositionProperty(list, func, 0)
which previously only started trying to find an item inlist
for whichfunc
returnstrue
fromlist[2]
onwards. This contradicts both the documentation and is not consistent with howPosition
behaves.