Skip to content

Conversation

@wojpok
Copy link
Collaborator

@wojpok wojpok commented Nov 4, 2025

This PR is a part of Pretty-printer PR #256 . This module have been separated for easier review. Interface is not complete and might be expanded in future. This version provides minimal required set of utilities to run pretty-printer.

@wojpok wojpok marked this pull request as ready for review November 5, 2025 09:57
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.

The implementation looks solid. I have some minor comments regarding names of some functions, and documentation comments. A also would consider making type parameter X named for all functions through parameter mechanism.

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

wojpok commented Nov 13, 2025

Changes:

  • All functions are annotated
  • changed the name of bind function to concatMap for compatibility with List module
  • pureLazy is pure. Documentation reflects that
  • Fixed documentation

Please, verify formatting style that I have used. Type annotation required me to break lines and I am not sure if this tyle is right.

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.

As you have asked, I wrote some comments about formatting. And there is still one unresolved conversation from my previous review.

@ppolesiuk ppolesiuk requested a review from Copilot November 15, 2025 20:49
Copilot finished reviewing on behalf of ppolesiuk November 15, 2025 20:52
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 introduces a new Stream module for lazy lists (also known as streams) to the standard library. This is part of the larger Pretty-printer PR #256 and provides the minimal required utilities for the pretty-printer implementation.

Key changes:

  • Adds lib/Stream.fram with lazy stream implementation including constructors, transformations, and folds
  • Adds comprehensive test coverage in test/stdlib/stdlib0007_Stream.fram
  • Extends lib/Lazy.fram with pureRef function and improved pureLazy documentation for better integration with streams

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

File Description
lib/Stream.fram Implements lazy stream data structure with core operations (construction, mapping, filtering, folding) and proper laziness semantics
test/stdlib/stdlib0007_Stream.fram Provides test coverage for stream operations including conversions, transformations, and edge cases
lib/Lazy.fram Adds pure reference constructor and improves documentation for pureLazy to support stream implementation

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

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, looks better. I have some minor comments, and Copilot have found some spelling errors in the documentation comments.

@wojpok
Copy link
Collaborator Author

wojpok commented Nov 17, 2025

For consistency, I changed the pattern matching order. Nil / empty list is always the first case in pattern matching

@ppolesiuk ppolesiuk requested a review from Copilot November 19, 2025 20:58
Copilot finished reviewing on behalf of ppolesiuk November 19, 2025 21:00
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

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


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

Comment on lines +46 to +48
Initializes lazy stream by iteratively applying function `f` on previous results.

This function may generate infinite stream.
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The documentation states 'iteratively applying function f on previous results' but the implementation uses unfold which applies f to the seed/state, not to previous results. Consider clarifying as 'by repeatedly applying function f to a state value' or 'by unfolding a state with function f'.

Suggested change
Initializes lazy stream by iteratively applying function `f` on previous results.
This function may generate infinite stream.
Initializes a lazy stream by repeatedly applying function `f` to a state value.
This function may generate an infinite stream.

Copilot uses AI. Check for mistakes.
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.

The PR is ready for merge. Thanks! The last comments from Copilot can be ignored, especially that on left to right direction of foldRight. It's a complete nonsense.

@ppolesiuk ppolesiuk merged commit a3e32b9 into fram-lang:master Nov 19, 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.

3 participants