Splitting seq.fs #2598
Merged
Splitting seq.fs #2598
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Don't polute surface area
Contributor
|
@jack-pappas --- in principle I like this split regardless of the outcome of iseq. It will take a while to review it I'm afraid. Kevin |
KevinRansom
reviewed
Mar 13, 2017
| member x.Dispose() = () | ||
|
|
||
| let Empty<'T> () = (new EmptyEnumerator<'T>() :> IEnumerator<'T>) | ||
| module Internal = |
Contributor
There was a problem hiding this comment.
This is a bit idiosyncratic ...
perhaps if you renamed the namespace containing SeqCore to Microsoft.FSharp.Collections.SeqCore and opened Microsoft.FSharp.Collections.SeqCore.IEnumerable? Then maybe not jam in a new module.
Contributor
Author
There was a problem hiding this comment.
Just trying to minimize lines code changed; maybe module naming could at least serve as as a temporary names until this is consumed?
Contributor
There was a problem hiding this comment.
I'm OK with this as a transitional step in rearranging the code with minimal diff
dsyme
approved these changes
Mar 14, 2017
cloudRoutine
pushed a commit
to cloudRoutine/fsharp
that referenced
this pull request
Mar 19, 2017
* Split seq.fs(i) in preperation for iseq * Made mkConcatSeq internal Don't polute surface area
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As proposed in this comment this should be a prelude to that #2587 - if this PR is merged and #2587 rebased then it should be provide a simpler set of diffs.
No functionality or change in logic is modified with this PR, it is just a split of the code that is from the
seq.fs(i)files.This has some benefit even without #2587 as it provides a physical separation of functionality that is used by the compiler for implementation of seq computational expressions that is shared with the Seq module.