Skip to content

Refactor core logic#7

Merged
iskandr merged 6 commits intomasterfrom
refactor_core_logic
Feb 13, 2015
Merged

Refactor core logic#7
iskandr merged 6 commits intomasterfrom
refactor_core_logic

Conversation

@iskandr
Copy link
Copy Markdown
Contributor

@iskandr iskandr commented Feb 10, 2015

No description provided.

Comment thread varcode/common.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd imagine you're doing this instead of using itertools.group_by because that one returns generators instead of lists? As an aside, this is a great place to use a collections.defaultdictionary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. Yep, I didn't want the small annoyance of converting the iterator to a dictionary at the call site.

  2. Good point about defaultdict!

@timodonnell
Copy link
Copy Markdown
Contributor

having never looked at this code before, I'm not sure if my feedback is very helpful, but besides my small nit comments, lgtm

@iskandr iskandr force-pushed the refactor_core_logic branch from 6a93ea5 to 013009e Compare February 13, 2015 17:13
@timodonnell
Copy link
Copy Markdown
Contributor

LGTM

iskandr added a commit that referenced this pull request Feb 13, 2015
@iskandr iskandr merged commit 782d5fd into master Feb 13, 2015
@iskandr iskandr deleted the refactor_core_logic branch February 13, 2015 17:44
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