Skip to content

Conversation

@wojpok
Copy link
Collaborator

@wojpok wojpok commented Nov 3, 2025

This code is part of a pretty-printer PR #256 . This code have been separated from main PR for easier review and merging.

lib/List.fram Outdated
Comment on lines 64 to 73
let rec dropLastAux xs =
match xs with
| [] => impossible ()
| [x] => []
| x :: xs => x :: dropLastAux xs
end in
if isEmpty xs then
None
else
Some (dropLastAux xs)
Copy link
Member

Choose a reason for hiding this comment

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

The invariant that dropLastAux works on non-empty list can be easily ensured by passing the head and the tail of the list separately. In my opinion such an implementation is more clear and doesn't use suspicious impossible function. Same comment applies to other last-related functions, as well as foldRight1* functions.

Suggested change
let rec dropLastAux xs =
match xs with
| [] => impossible ()
| [x] => []
| x :: xs => x :: dropLastAux xs
end in
if isEmpty xs then
None
else
Some (dropLastAux xs)
let rec dropLastAux y xs =
match xs with
| [] => []
| x :: xs => y :: dropLastAux x xs
end in
match xs with
| [] => None
| x :: xs => Some (dropLastAux x xs)
end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool trick. I will apply that

@wojpok wojpok requested a review from ppolesiuk November 4, 2025 11:58
Copy link
Member

@ppolesiuk ppolesiuk left a comment

Choose a reason for hiding this comment

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

Now, the code looks good. I propose some minor fixes in documentation comments, and I would prefer to be consistent in the order of pattern-matching clauses: some functions (like last) match with the empty list first, but the others (e.g., dropLast) put this clause as the last one. If these things will be fixed, I will opt for merging this PR.

lib/List.fram Outdated
in if n < 0 then ~onError () else nthErrAux xs n

{##
Returns the last element of a list or None if the list is empty.
Copy link
Member

Choose a reason for hiding this comment

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

None could be quoted, to be consistent with previous documentation comments.

Suggested change
Returns the last element of a list or None if the list is empty.
Returns the last element of a list or `None` if the list is empty.

lib/List.fram Outdated
end

{##
`foldLeft f [x1, x2, ... ,xn]` is `f (... (f (f x1 x2) x3) ...) xn`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`foldLeft f [x1, x2, ... ,xn]` is `f (... (f (f x1 x2) x3) ...) xn`.
`foldLeft1 f [x1, x2, ... ,xn]` is `f (... (f (f x1 x2) x3) ...) xn`.

lib/List.fram Outdated
end

{##
`foldLeft f [x1, x2, ... ,xn]` is `f (... (f (f x1 x2) x3) ...) xn`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`foldLeft f [x1, x2, ... ,xn]` is `f (... (f (f x1 x2) x3) ...) xn`.
`foldLeft1Err f [x1, x2, ... ,xn]` is `f (... (f (f x1 x2) x3) ...) xn`.

lib/List.fram Outdated
end

{##
`foldRight f [x1, x2, ..., xn]` is `f x1 (... (f x(n-1) xn)))`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`foldRight f [x1, x2, ..., xn]` is `f x1 (... (f x(n-1) xn)))`.
`foldRight1 f [x1, x2, ..., xn]` is `f x1 (... (f x(n-1) xn)))`.

lib/List.fram Outdated
end

{##
`foldRight f [x1, x2, ..., xn]` is `f x1 (... (f x(n-1) xn)))`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`foldRight f [x1, x2, ..., xn]` is `f x1 (... (f x(n-1) xn)))`.
`foldRight1Err f [x1, x2, ..., xn]` is `f x1 (... (f x(n-1) xn)))`.

@wojpok wojpok requested a review from ppolesiuk November 11, 2025 16:50
@wojpok
Copy link
Collaborator Author

wojpok commented Nov 11, 2025

All pattern matching begins now with an empty list

Copy link
Member

@ppolesiuk ppolesiuk 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. Thanks!

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the List module with new functions for operating on the last elements of lists and for folding operations that don't require an initial accumulator. The changes introduce functions with both Option-based and error-handling variants.

  • Adds functions for operating on the last element of lists (dropLast, last, dropTakeLast)
  • Introduces fold operations without initial values (foldLeft1, foldRight1)
  • Provides both Option-returning and error-handling (Err) variants for all new functions

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
lib/List.fram Implements new list functions and exposes them as methods on lists
test/stdlib/stdlib0003_List.fram Adds comprehensive test coverage for all new list functions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ppolesiuk ppolesiuk merged commit d73b816 into fram-lang:master Nov 11, 2025
7 checks passed
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.

2 participants