search

twitter / summingbird

Streaming MapReduce with Scalding and Storm
https://twitter.com/summingbird
Apache License 2.0
2.14k stars 267 forks source link

Add doc comments for Dag and minor reformatting. #683

Closed pankajroark closed 8 years ago

johnynek commented 8 years ago

seems like a good improvement to style and documentation.

you might contrast this to the code in https://github.com/twitter/summingbird/tree/develop/summingbird-core/src/main/scala/com/twitter/summingbird/graph which are utilities on generic graphs, which we use to inspect the producer graph generally (like to reverse the graph to see downstream dependencies).

pankajroark commented 8 years ago

@johnynek Thanks for pointing out docs for other graph utilities. While reading Dag optimization related code I came across several places where I felt documentation would have been really useful and expected it. I'm trying to add docs to those places, will likely send many more such PRs. Let me know when it feels I'm going overboard :). Can I consider your previous comment a thumbs up? Praneeth already reviewed it and I'm ready to merge it.

johnynek commented 8 years ago

👍

pankajroark commented 8 years ago

Merged from command line, since I needed to squash commits.

johnynek commented 8 years ago

note that github can do squash commits now. After you click the merge button you can select squash.

On Thu, Aug 18, 2016 at 8:52 AM, Pankaj Gupta notifications@github.com wrote:

Merged from command line, since I needed to squash commits.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/twitter/summingbird/pull/683#issuecomment-240820467, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdlew004yIrlgVpHdCBoWR4YjMF4Tks5qhKn1gaJpZM4Jm8do .

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

pankajroark commented 8 years ago

Ahh... didn't know that merge button gives squash option. Will do that from next time.