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

Fix infinite loop in CountingHash gzipped IO #1043

Merged
merged 6 commits into from
Jun 14, 2015
Merged

Conversation

kdm9
Copy link
Contributor

@kdm9 kdm9 commented May 29, 2015

Addresses #1038

  • Is it mergeable?
  • Did it pass the tests?
  • If it introduces new functionality in scripts/ is it tested?
    Check for code coverage with make clean diff-cover
  • Is it well formatted? Look at make pep8, make diff_pylint_report,
    make cppcheck, and make doc output. Use make format and manual
    fixing as needed.
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Is it documented in the ChangeLog?
    http://en.wikipedia.org/wiki/Changelog#Format
  • Was a spellchecker run on the source code and documentation after
    changes were made?
  • Is the Copyright year up to date?

@kdm9
Copy link
Contributor Author

kdm9 commented May 29, 2015

I'm going to add a test of this, but it will take a while to run and at least 2gb RAM. I'll use @attr('huge') to mark it. Is this OK?

kdm9 added a commit that referenced this pull request May 29, 2015
@kdm9
Copy link
Contributor Author

kdm9 commented May 29, 2015

Lines's stability is 87.83 and set mininum stability is 87.84. Soooo close!

Also, ready for review @ctb @mr-c

@ctb
Copy link
Member

ctb commented May 29, 2015

LGTM on a skim, but I will leave for @mr-c to merge. Note that we are in the middle of moving across the country so it may take a few weeks!

@kdm9
Copy link
Contributor Author

kdm9 commented May 29, 2015

Thanks, and good luck with the move :)

@mr-c
Copy link
Contributor

mr-c commented May 29, 2015

@kdmurray91 'huge' is what 'linux' should have been named; can you rename the existing @attr('linux') to huge?

@kdm9
Copy link
Contributor Author

kdm9 commented May 30, 2015

No longer mergable, but as you're all moving I'll only rebase to fix the conflicts when you're ready to merge, which may take some time. Ping me when you're ready, and I'll rebase immediately :)

@ctb
Copy link
Member

ctb commented May 30, 2015 via email

@ctb ctb added this to the 2.0 milestone Jun 6, 2015
@mr-c
Copy link
Contributor

mr-c commented Jun 8, 2015

  • update linux->huge in Jenkins, Makefile (mr-c)

@mr-c
Copy link
Contributor

mr-c commented Jun 8, 2015

also update instructions in docs

@kdm9
Copy link
Contributor Author

kdm9 commented Jun 9, 2015

yo @mr-c, just let me know as soon as you need anything from me on this. Happy to implement whatever else you need.

@mr-c
Copy link
Contributor

mr-c commented Jun 14, 2015

@kdmurray91 The worst of the merges should be done for a while. It you want to make this mergeable that'd be appreciated :-)

@kdm9 kdm9 force-pushed the fix/gzip-infinite-loop branch from 235f2a5 to fa49dc7 Compare June 14, 2015 09:28
@kdm9
Copy link
Contributor Author

kdm9 commented Jun 14, 2015

Good to go!

@mr-c
Copy link
Contributor

mr-c commented Jun 14, 2015

LGTM, Thanks @kdmurray91 !

mr-c added a commit that referenced this pull request Jun 14, 2015
Fix infinite loop in CountingHash gzipped IO
@mr-c mr-c merged commit ba7c760 into master Jun 14, 2015
@mr-c mr-c deleted the fix/gzip-infinite-loop branch June 14, 2015 09:36
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