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

class-ify ShuffleRegionJoin, force setting seqdict #698

Merged
merged 1 commit into from Jun 3, 2015

Conversation

Projects
None yet
2 participants
@ryan-williams
Member

ryan-williams commented Jun 2, 2015

fixes #697

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jun 2, 2015

Member

As per my comments on #697, I am +1. However, I'm going to hold off on merging this until we cut a 0.17.0 release, and to give @laserson and @tdanford more time to comment.

Member

fnothaft commented Jun 2, 2015

As per my comments on #697, I am +1. However, I'm going to hold off on merging this until we cut a 0.17.0 release, and to give @laserson and @tdanford more time to comment.

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Jun 2, 2015

Member

Yea we can discuss more on #697 for the time being…

Member

ryan-williams commented Jun 2, 2015

Yea we can discuss more on #697 for the time being…

@ryan-williams ryan-williams changed the title from kill RegionJoin supertrait, narrow Shuffle API to class-ify ShuffleRegionJoin, force setting seqdict Jun 2, 2015

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Jun 2, 2015

Member

updated this and the description to reflect that it is no longer removing the RegionJoin trait, but simply forcing ShuffleRegionJoin to be initialized with a seqDict and partitionSize.

Member

ryan-williams commented Jun 2, 2015

updated this and the description to reflect that it is no longer removing the RegionJoin trait, but simply forcing ShuffleRegionJoin to be initialized with a seqDict and partitionSize.

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Jun 3, 2015

Member

this is ready to go, yea @fnothaft? (don't want to pester you while you are at a conference; should I enlist others to merge)?

Member

ryan-williams commented Jun 3, 2015

this is ready to go, yea @fnothaft? (don't want to pester you while you are at a conference; should I enlist others to merge)?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jun 3, 2015

Member

I'm about to run to the airport but will have an hour or two there where I would be glad to merge this. Is this rebased on ToT? If not, I'll merge manually.

Member

fnothaft commented Jun 3, 2015

I'm about to run to the airport but will have an hour or two there where I would be glad to merge this. Is this rebased on ToT? If not, I'll merge manually.

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Jun 3, 2015

Member

I've got the green "Merge pull request" button here so it is either at ToT or close enough. Update: it is in fact at ToT. (In general if it's close enough that it merges cleanly, do you still care about rebasing?)

Member

ryan-williams commented Jun 3, 2015

I've got the green "Merge pull request" button here so it is either at ToT or close enough. Update: it is in fact at ToT. (In general if it's close enough that it merges cleanly, do you still care about rebasing?)

fnothaft added a commit that referenced this pull request Jun 3, 2015

Merge pull request #698 from ryan-williams/regjoin
class-ify ShuffleRegionJoin, force setting seqdict

@fnothaft fnothaft merged commit 3295cd8 into bigdatagenomics:master Jun 3, 2015

1 check passed

default Merged build finished.
Details
@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jun 3, 2015

Member

Merged! Thanks @ryan-williams!

In general if it's close enough that it merges cleanly, do you still care about rebasing?

Yeah, we prefer everything to be rebased before merging in. If all PRs are rebased on ToT before they are merged, we keep a linear commit history. This is IMO a funky design decision in git... In any case, if a PR is small and is not rebased on ToT, we'll manually rebase and merge it.

Member

fnothaft commented Jun 3, 2015

Merged! Thanks @ryan-williams!

In general if it's close enough that it merges cleanly, do you still care about rebasing?

Yeah, we prefer everything to be rebased before merging in. If all PRs are rebased on ToT before they are merged, we keep a linear commit history. This is IMO a funky design decision in git... In any case, if a PR is small and is not rebased on ToT, we'll manually rebase and merge it.

@ryan-williams ryan-williams deleted the ryan-williams:regjoin branch Jun 3, 2015

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Jun 3, 2015

Member

Cool, makes sense.

This is IMO a funky design decision in git

The allowing of non-linear commit histories, you mean?

Member

ryan-williams commented Jun 3, 2015

Cool, makes sense.

This is IMO a funky design decision in git

The allowing of non-linear commit histories, you mean?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jun 3, 2015

Member

The allowing of non-linear commit histories, you mean?

Yeah. I mean, obviously this is personal preference but I can't fathom a reason that you'd want a non-linear commit history.

There are also funky design decisions in Github. E.g., the "is this rebased?" question would go away completely if Github exposed the option to only allow fast forward merges. But—god knows why—Github goes the opposite way and disallows fast forward merges via the merge button...

Anyways, this is frankly personal preference. I mostly love git and Github. ;)

Member

fnothaft commented Jun 3, 2015

The allowing of non-linear commit histories, you mean?

Yeah. I mean, obviously this is personal preference but I can't fathom a reason that you'd want a non-linear commit history.

There are also funky design decisions in Github. E.g., the "is this rebased?" question would go away completely if Github exposed the option to only allow fast forward merges. But—god knows why—Github goes the opposite way and disallows fast forward merges via the merge button...

Anyways, this is frankly personal preference. I mostly love git and Github. ;)

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Jun 3, 2015

Member

Makes sense; I've slowly come to want to embrace branching/merging over squashing/rebasing, but it's been a process and taken work.

Github disallowing fast-forward merges, interesting… I guess it doesn't really make sense to merge something that hasn't really been branched (e.g. is an ancestor of HEAD)? Good to know…

Member

ryan-williams commented Jun 3, 2015

Makes sense; I've slowly come to want to embrace branching/merging over squashing/rebasing, but it's been a process and taken work.

Github disallowing fast-forward merges, interesting… I guess it doesn't really make sense to merge something that hasn't really been branched (e.g. is an ancestor of HEAD)? Good to know…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment