feat(Array): add dropLast and dropRight functions#724
Conversation
- dropLast removes last element, returns empty if empty - dropRight removes N elements from end, clamps safely - Export both functions in module header Closes neohaskell#704
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Array API adds ChangesArray drop operations
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
NickSeagull
left a comment
NickSeagull
left a comment
There was a problem hiding this comment.
Hey, thanks for the PR!
Can you ensure the doc comments are in english, and also include an extension of the Array test suite for this change? Thanks!
| dropLast :: Array a -> Array a | ||
| dropLast arr@(Array vector) | ||
| | Data.Vector.null vector = arr | ||
| | otherwise = Array (Data.Vector.init vector) |
There was a problem hiding this comment.
Can you use the existing functions in this module rather than wrapping unwrapping?
Also, do not use guards, we use if..then..else
| dropRight n arr = | ||
| let len = length arr | ||
| keep = max 0 (len - n) | ||
| in if keep == len | ||
| then arr | ||
| else Array (Data.Vector.take keep (unwrap arr)) |
There was a problem hiding this comment.
use do...let instead of let..in. Also, use the other functions when possible
There was a problem hiding this comment.
oops, sry 😅 i'll fix it up. sorry to bother you
There was a problem hiding this comment.
not bothering at all! thanks!
|
i've optimized the code and added tests based on the reviewer's feedback. pls review again😸️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/core/Array.hs`:
- Line 587: Replace the single-letter type variable a with a descriptive
multi-letter variable such as element in the dropLast signature at
core/core/Array.hs lines 587-587 and the dropRight signature at
core/core/Array.hs lines 604-604, updating each corresponding type declaration
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d552b189-7d4e-4573-baa1-31c2c46d489e
📒 Files selected for processing (2)
core/core/Array.hscore/test/ArraySpec.hs
|
@XiayiZhang thanks a lot for the changes! One last request, can you run It comes from GitHub Actions, it is a step that we use to generate contextual information for the agents to work more efficiently. |
NickSeagull
left a comment
NickSeagull
left a comment
There was a problem hiding this comment.
This looks great now! Can you run ./dev codemap and push the changes so that CICD can run? I will merge afterwards.
2 participants
(issue feat(Array): add dropLast / dropRight — Array has no "drop from the end" #704)
Summary by CodeRabbit
New Features
dropLastto remove the final element from an array.dropRightto remove the lastnelements from an array.n.Tests
dropLastanddropRight, including edge cases.