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

Fix PositionProperty with from < 1 #2056

Merged
merged 1 commit into from
Jan 8, 2018

Conversation

james-d-mitchell
Copy link
Contributor

@james-d-mitchell james-d-mitchell commented Jan 1, 2018

This PR fixes a bug in PositionProperty(list, func, 0) which previously only started trying to find an item in list for which func returns true from list[2] onwards. This contradicts both the documentation and is not consistent with how Position behaves.

@fingolfin
Copy link
Member

I think you mean PositionProperty everywhere? (Including the commit message, which thus should be adjusted) ?

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them topic: library labels Jan 2, 2018
@fingolfin
Copy link
Member

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;
Copy link
Member

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.

@fingolfin
Copy link
Member

For cream with a cherry on top, perhaps also add a test case? :-)

@james-d-mitchell
Copy link
Contributor Author

Done, with cream and a cherry on top.

@fingolfin fingolfin changed the title Fix PositionsProperty with from < 1 Fix PositionProperty with from < 1 Jan 3, 2018
Copy link
Member

@fingolfin fingolfin left a 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?

@james-d-mitchell
Copy link
Contributor Author

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?

@fingolfin
Copy link
Member

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 master) -- ideally in that order, because retargeting a PR does not trigger a Travis rebuild. Could you do that, please, @james-d-mitchell ?

@ChrisJefferson
Copy link
Contributor

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.

@fingolfin fingolfin changed the base branch from master to stable-4.9 January 6, 2018 18:52
@fingolfin
Copy link
Member

I changed the base to stable-4.9, but now @james-d-mitchell needs to rebase

@fingolfin fingolfin merged commit 5d38117 into gap-system:stable-4.9 Jan 8, 2018
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Jan 21, 2018
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants