-
Notifications
You must be signed in to change notification settings - Fork 25
Prelude extension #268
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
Prelude extension #268
Conversation
ppolesiuk
left a comment
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.
Looks good. I would consider defining min and max functions for any type with comparison.
lib/Prelude.fram
Outdated
| pub let max (x : Int) (y : Int) = | ||
| if x > y then x else y |
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.
Is there any reason to define this function for Int type only? Maybe we can define a more polymorphic version for any type with lt method.
| pub let max (x : Int) (y : Int) = | |
| if x > y then x else y | |
| pub let max {T, method lt : T -> T ->> Bool} (x : T) (y : T) = | |
| if x < y then y else x |
BTW, we could add also min function, and provide some documentation for them.
3237b3a to
3de7531
Compare
lib/Prelude.fram
Outdated
| pub let min {T, method gt : T -> T ->> Bool} (x : T) (y : T) = | ||
| if x > y then y else x |
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.
Is there any reason to implement min and max functions on top of different comparison methods (lt and gt, respectively)? Both implementation can be based on lt method.
| pub let min {T, method gt : T -> T ->> Bool} (x : T) (y : T) = | |
| if x > y then y else x | |
| pub let min {T, method lt : T -> T ->> Bool} (x : T) (y : T) = | |
| if x < y then x else y |
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.
I thought that it would be more accurate to return the first element when elements are equal. It may be not necessary. I will remove this later
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.
| pub let min {T, method gt : T -> T ->> Bool} (x : T) (y : T) = | |
| if x > y then y else x | |
| pub let min {T, method gt : T -> T ->> Bool} (x : T) (y : T) = | |
| if y < x then y else x |
?
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.
Yeah, right, my bad 😅
|
Both |
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.
Pull Request Overview
This PR extends the Prelude library with additional utility functions and methods, supporting issue #256.
- Adds
minandmaxfunctions for comparable types - Adds
strListCatfor efficient string concatenation andreplicatefor string repetition - Exposes
fstandsndas tuple methods - Adds
mapmethod to Option type
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/Prelude.fram | Adds fst/snd methods, min/max comparison functions, strListCat extern, and replicate string utility |
| lib/Base/Option.fram | Adds map method for functional transformation of Option values |
| test/stdlib/stdlib0006_Prelude.fram | New test file covering min, max, strListCat, and replicate functions |
| test/stdlib/stdlib0001_Option.fram | Adds test case for the new map method on Option type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Copilot have found some minor mistakes in comments, and its suggestions looks reasonable. If these things will be fixed, the PR will be ready for merge. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I have applied all suggestions directly |
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (let s' = replicate s (n / 2) in | ||
| s' + s') | ||
| else | ||
| (let s' = replicate s (n / 2) in |
Copilot
AI
Nov 13, 2025
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.
Inconsistent indentation: this line uses one space for indentation while the previous code uses two spaces. Should be indented with two spaces to match the rest of the file.
| (let s' = replicate s (n / 2) in | |
| s' + s') | |
| else | |
| (let s' = replicate s (n / 2) in | |
| (let s' = replicate s (n / 2) in | |
| s' + s') | |
| else | |
| (let s' = replicate s (n / 2) in |
| (let s' = replicate s (n / 2) in | ||
| s' + s') | ||
| else | ||
| (let s' = replicate s (n / 2) in |
Copilot
AI
Nov 13, 2025
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.
Inconsistent indentation: this line uses one space for indentation while the previous code uses two spaces. Should be indented with two spaces to match the rest of the file.
| (let s' = replicate s (n / 2) in | |
| (let s' = replicate s (n / 2) in |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR is part of #256 separated for easier merging and review