Skip to content

do not drop extra elements in Data.List.Linear drop and take#486

Merged
aspiwack merged 4 commits intotweag:masterfrom
exaexa:patch-1
Apr 1, 2025
Merged

do not drop extra elements in Data.List.Linear drop and take#486
aspiwack merged 4 commits intotweag:masterfrom
exaexa:patch-1

Conversation

@exaexa
Copy link
Copy Markdown
Contributor

@exaexa exaexa commented Mar 28, 2025

Fixes #484 .

@exaexa
Copy link
Copy Markdown
Contributor Author

exaexa commented Mar 31, 2025

(I also pushed an update that fixes the errors reported by CI)

Copy link
Copy Markdown
Collaborator

@b-mehta b-mehta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for this, in test/Test/Data/List.hs?

@exaexa
Copy link
Copy Markdown
Contributor Author

exaexa commented Mar 31, 2025

@b-mehta Hi, I'm not sure about what the tests should look like (I'm not really good with linear haskell, fixed this basically as a passer-by on the IRC...), and unfortunately currently I don't have much time to investigate. Any help would be apprectiated.

cc @liamzee (who found this originally) could have some test material

@aspiwack
Copy link
Copy Markdown
Member

aspiwack commented Apr 1, 2025

I can do it, no worries. Thank all three of you @liamzee , @tomsmeding , @exaexa for finding the bug and coming up with a fix.

@aspiwack
Copy link
Copy Markdown
Member

aspiwack commented Apr 1, 2025

@b-mehta would you have a look at my tests and tell me if they make sense to you? If you'd like to see more?

Copy link
Copy Markdown
Collaborator

@b-mehta b-mehta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Copy Markdown
Member

@aspiwack aspiwack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, then! Let me approve. I'll merge as soon as CI passes.

Thanks @b-mehta for the review. It seems that you don't have write access, this is silly, in case you do more reviews like this in the future, I've sent you an invite with the appropriate permission.

@aspiwack aspiwack enabled auto-merge April 1, 2025 14:47
@aspiwack aspiwack merged commit a89eab5 into tweag:master Apr 1, 2025
6 checks passed
@aspiwack aspiwack mentioned this pull request Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data.List.Linear.{take,drop} take one list element too many

3 participants