Skip to content
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

Rebased and reorganized #223 for Jenkins #238

Merged
merged 5 commits into from
May 5, 2016
Merged

Rebased and reorganized #223 for Jenkins #238

merged 5 commits into from
May 5, 2016

Conversation

aantron
Copy link
Collaborator

@aantron aantron commented May 4, 2016

Discussion in #223. Resolves #223.

seliopou added 3 commits May 4, 2016 10:35
Before this commit, the Lwt_stream API provided no reliable way to
detect if a stream had been closed. While on_termination notified the
user if the stream was closed in the future, it did not tell the user
if the stream had been closed in the past.

The is_closed function addresses this problem, allowing the user to
detect if a stream has already been closed at any point in time.

The closed function returns a thread that will sleep until the stream
is closed.

Lwt_stream.on_termination is now implemented in terms of Lwt.closed.

Lwt_stream.on_termination will be deprecated and ultimately removed in
favor of Lwt_stream.closed.
The Lwt_stream module had multiple copies of low-level enqueueing code
of two varieties copied in serveral places. This commit moves one
variety of enqueing code into a single function called enqueue', and
implements the second variety as the function enqueue, in terms of
enqueue'.
@aantron aantron changed the title Squashed and reorganized #223 for Jenkins Rebased and reorganized #223 for Jenkins May 4, 2016
@aantron aantron force-pushed the seliopou-squash branch from ca7ffdc to 1ee1c84 Compare May 5, 2016 10:19
@aantron aantron merged commit 1ee1c84 into master May 5, 2016
@aantron aantron deleted the seliopou-squash branch May 5, 2016 10:43
@seliopou
Copy link
Contributor

seliopou commented May 9, 2016

Why were my commits rebased and modified? These commits now contain messages that I did not author, even though I'm listed as the author. Also, why not actually include a merge commit so there's a pointer back to discussion in the commit history?

This is also going to complicate merging #239.

@aantron
Copy link
Collaborator Author

aantron commented May 10, 2016

Everything in the commit messages is something you authored, only some commits were squashed into one, which has trivial sentence changes to make the sentences flow together after concatenation. The only sentence I added is the overall title, which is a natural consequence of a squash. I think you as author and me as committer reflects this situation accurately.

I did this while reviewing the code, because the original order, together with hidden comments in #223, made it rather difficult to clearly see what was going on, especially given that I was not yet as familiar with the surrounding code. Given that I had already done this squash for review, I decided to post and then merge it, without asking you to go through the exercise, in potentially multiple iterations with me, of cleaning up the messy history of #223 to achieve basically the same end result. While I now realize I should have clearly told you my intentions in a comment, there also was a link to this PR from #223 that was readily visible. Perhaps the title "...for Jenkins" was misleading? Anyway, I assumed that there had been time to object, so I apologize.

There are links back to the discussion. For example, e4f39b8 links to this PR, which links to #223. I grant that this isn't so useful for non-GH interfaces, so I suppose that I should prefer to merge using the GH web UI in the future.

For merging #239, I decided that I would rebase the relevant commits again myself. You are welcome to do this yourself or offer another course of action. Either way, I don't see what is so complicated – it is two git cherry-picks and a git push -f away from being all rebased.

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