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

BF: Fix import and write issues with openpyxl #1276

Merged
merged 4 commits into from Oct 16, 2016
Merged

Conversation

@janfreyberg
Copy link
Contributor

@janfreyberg janfreyberg commented Oct 15, 2016

  • not using ExcelWriter()
  • now compatible with latest openpyxl version

This closes #1274

- not using ExcelWriter()
- now compatible with latest openpyxl version
@codecov-io
Copy link

@codecov-io codecov-io commented Oct 15, 2016

Current coverage is 45.89% (diff: 66.66%)

Merging #1276 into master will decrease coverage by <.01%

@@             master      #1276   diff @@
==========================================
  Files           232        232          
  Lines         36819      36818     -1   
  Methods           0          0          
  Messages          0          0          
  Branches       5669       5670     +1   
==========================================
- Hits          16899      16896     -3   
- Misses        18374      18375     +1   
- Partials       1546       1547     +1   

Powered by Codecov. Last update a99df31...2eca7d6

@coveralls
Copy link

@coveralls coveralls commented Oct 15, 2016

Coverage Status

Coverage decreased (-0.004%) to 50.09% when pulling b64fe91 on janfreyberg:master into a99df31 on psychopy:master.

@@ -663,8 +667,6 @@ def saveAsExcel(self, fileName, sheetName='rawData',
wb.properties.creator = 'PsychoPy' + psychopy.__version__
newWorkbook = True

ew = ExcelWriter(workbook=wb)

This comment has been minimized.

@hoechenberger

hoechenberger Oct 15, 2016
Member

Are you sure we don't need the ExcelWriter for openpyxl < 2.4?

This comment has been minimized.

@janfreyberg

janfreyberg Oct 15, 2016
Author Contributor

Fairly sure. It worked with openpyxl 2.3.2, which is what I had before - I think workbook.save() has been in the package for a while.

This comment has been minimized.

@hoechenberger

hoechenberger Oct 15, 2016
Member

Okay nice, thanks for checking this!


try:
# import openpyxl
import openpyxl
from openpyxl.cell import get_column_letter

This comment has been minimized.

@hoechenberger

hoechenberger Oct 15, 2016
Member

I like this explicit approach, makes it very clear what's going on and why.

@hoechenberger hoechenberger self-assigned this Oct 15, 2016
@hoechenberger hoechenberger changed the title Fix import and write issues with openpyxl BF: Fix import and write issues with openpyxl Oct 15, 2016
Copy link
Member

@hoechenberger hoechenberger left a comment

Looks good to me! I would like to request one small change, though:

Ideally, to make it easier to assemble the change log and pick commits for the next release, the commit summary line would start with a descriptor specifying the type of change(s) introduced. In this case, it would be "BF: " for bug fix. So the commit summary line should read something like:
BF: Fix import and write issues with openpyxl

To change the commit message, you have to do an interactive rebase on the upstream master branch, reword that commit, and force-push it back into your branch. If you don't have any clue what the heck I'm talking about (but are eager to learn), just contact me at richard.hoechenberger@gmail.com. If you don't feel like doing this weird stuff, we might as well just leave everything as it is now, and pull in the unmodified change set. ;)

@janfreyberg
Copy link
Contributor Author

@janfreyberg janfreyberg commented Oct 15, 2016

I think I've just done that, but I'm not sure - I used the git commit --amend command, so there's a new commit in my branch - I'm not sure why there's an empty "Merge branch 'master'" commit in there, as well though.

@coveralls
Copy link

@coveralls coveralls commented Oct 15, 2016

Coverage Status

Coverage decreased (-0.004%) to 50.09% when pulling 7062c3e on janfreyberg:master into a99df31 on psychopy:master.

@hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Oct 15, 2016

Hehe no worries, I think we can just squash these commits.

The proper way to simply rename/change a commit, without creating a new one, is via interactive rebase, though. But never mind, all is good.

Maybe a little advice for the future: it's always a good idea to create a new, separate branch for fixes and new features, and not work on master. Makes handling easier. My workflow is usually:

  • pull in all changes from the psychopy upstream master branch into my local master branch
  • create a new feature branch off of this local master branch
  • work on this branch, commit to that branch
  • once I'm done developing, switch to the master branch and pull in any changes from upstream master that possibly occurred while I'd been busy working on my own stuff
  • switch back to my feature branch
  • rebase that branch on my local master, possibly rewording, re-arranging, and squashing commits
  • push to GitHub (or even force-push, if I had previously pushed and rebased afterwards)
  • file a PR for that branch
  • work on something else (i.e., new feature branch based on master) while my PR is awaiting approval
  • once the PR gets accepted and merged, delete the feature branch both locally and on GitHub

... and the cycle continues

  • pull in upstream changes to local master
  • rebase new feature branch on master
  • ...

Maybe there's more elegant ways, but this is how I usually work. I don't use the command line anymore btw, there's great GUI tools available now. And my IDE also does most (actually, all) of that stuff anyway... (I'm using PyCharm)

At any rate, thank you very much for your contribution!

@janfreyberg
Copy link
Contributor Author

@janfreyberg janfreyberg commented Oct 15, 2016

OK, thanks for the pointers! That seems like a good workflow, I'll keep that in mind for the future. And I will explore some of the GUI tools... I code in Atom, which does seem to have some GUI extensions that look useful.

@hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Oct 15, 2016

I hear GitKraken is a neat GUI, maybe you can check it out: https://www.gitkraken.com/features

Update to latest from master
@coveralls
Copy link

@coveralls coveralls commented Oct 15, 2016

Coverage Status

Coverage decreased (-0.004%) to 50.09% when pulling 2eca7d6 on janfreyberg:master into a99df31 on psychopy:master.

@peircej peircej merged commit f8bcd36 into psychopy:master Oct 16, 2016
2 checks passed
2 checks passed
code-quality/landscape Code quality increased by 0.02%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants