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

Fixed abagen.mouse column ordering #62

Merged
merged 2 commits into from
Jul 9, 2019
Merged

Conversation

abkosar
Copy link
Contributor

@abkosar abkosar commented Jul 9, 2019

#56

@abkosar
Copy link
Contributor Author

abkosar commented Jul 9, 2019

Hey @rmarkello -- I fixed the issue with your proposal and I also added an assert statement to the tests you already had that compares the column ordering to entries. Let me know if this works or else I can change it to the way you want.

@coveralls
Copy link

coveralls commented Jul 9, 2019

Pull Request Test Coverage Report for Build 226

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 97.179%

Totals Coverage Status
Change from base Build 219: 0.009%
Covered Lines: 930
Relevant Lines: 957

💛 - Coveralls

Copy link
Owner

@rmarkello rmarkello left a comment

Choose a reason for hiding this comment

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

Hi @abkosar ! This looks great—thank you so much!

I made one suggestion that I think should fix the Travis failure; it's a linting error caused by flake8 expecting two blank line before a function definition in test_io.py. If you want to accept the suggestion then the tests should re-run and this should all be perfect!

Thank you again 😁

abagen/mouse/tests/test_io.py Show resolved Hide resolved
Copy link
Contributor Author

@abkosar abkosar left a comment

Choose a reason for hiding this comment

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

Hey @rmarkello! Looks great, yeah I wasn't paying attention to spacing.

@rmarkello
Copy link
Owner

Hey @abkosar: if you could accept the suggestion to add the blank line that would be great! I'm not able to do it myself, but as soon as you do I'll go ahead and merge.

Thanks again!

Co-Authored-By: Ross Markello <rossmarkello@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #62 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   97.16%   97.17%   +<.01%     
==========================================
  Files          24       24              
  Lines         954      957       +3     
==========================================
+ Hits          927      930       +3     
  Misses         27       27
Impacted Files Coverage Δ
abagen/mouse/tests/test_io.py 100% <100%> (ø) ⬆️
abagen/mouse/io.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc80ff6...b56580f. Read the comment docs.

1 similar comment
@codecov-io
Copy link

Codecov Report

Merging #62 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   97.16%   97.17%   +<.01%     
==========================================
  Files          24       24              
  Lines         954      957       +3     
==========================================
+ Hits          927      930       +3     
  Misses         27       27
Impacted Files Coverage Δ
abagen/mouse/tests/test_io.py 100% <100%> (ø) ⬆️
abagen/mouse/io.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc80ff6...b56580f. Read the comment docs.

@rmarkello rmarkello merged commit b6f1e0b into rmarkello:master Jul 9, 2019
@abkosar
Copy link
Contributor Author

abkosar commented Jul 9, 2019

Hey @rmarkello -- I think everything looks good!

@rmarkello
Copy link
Owner

This was great ! Thanks again so much for the contribution, @abkosar !

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.

4 participants