Skip to content

I copied the code from the documentation and adjusted it to make it run on my computer #35

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dmr
Copy link

@dmr dmr commented Apr 7, 2014

There was no recordset in the example on https://python-docx.readthedocs.org/en/latest/ so I created it. Do you want to include it?

@scanny scanny modified the milestones: v0.6.0, 0.6.2 May 1, 2014
@scanny scanny modified the milestones: later, 0.6.2 May 13, 2014
@scanny scanny removed this from the later milestone Apr 9, 2016
@Apteryks
Copy link
Contributor

Not sure what this is supposed to add to the example. I only see a list of dictionaries being used as a list of dictionaries, which doesn't have much to do with python-docx. I'd suggest we close this unless I'm missing something.

@scanny
Copy link
Contributor

scanny commented Sep 14, 2016

Yes, I'm not sure I would demonstrate it quite this way, but I think it's a legitimate complaint about the example. Every so often we get someone mentioning that the example doesn't work as stated. It's invariably a beginner, but it seems reasonable to me we should have an example that works when cut and pasted, just so someone starting out can get some immediate gratification :)

@dmr
Copy link
Author

dmr commented Sep 21, 2016

So what can I do here? Is there anything to fix or will you merge? This is a really old thread...

@Apteryks
Copy link
Contributor

Apteryks commented Sep 22, 2016

Hello drm, sorry about my last message I must have been too tired to see through. You did fix issues within the example code (which apparently is not used to generate the rendered image of the example). You shouldn't have removed the Inches class import though, as it is used for setting the picture width.

I generated the demo.docx file, but only have LibreOffice to visualize the result (which differs a bit -- for example the table's style seems to not have been applied). It'd be nice that the image be generated at the time we build the documentation, but for now I'd propose that dmr's change be integrated and this issue closed (with the exception of the Inches import line removal).

Copy link
Contributor

@Apteryks Apteryks left a comment

Choose a reason for hiding this comment

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

Only change requested: leave the imports as they were.

@@ -18,10 +18,8 @@ Here's an example of what |docx| can do:
|img| ::

from docx import Document
from docx.shared import Inches
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though it works without it (I'm not sure how?), we use Inches in the example. I'd suggest we leave it there, because "explicit is better than implicit".

@@ -46,11 +44,16 @@ Here's an example of what |docx| can do:
hdr_cells[0].text = 'Qty'
hdr_cells[1].text = 'Id'
hdr_cells[2].text = 'Desc'
recordset = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Good! Not sure where recordset came from before originally.

@dmr
Copy link
Author

dmr commented Nov 9, 2016

Corrected the import

@Apteryks
Copy link
Contributor

Apteryks commented Dec 2, 2016

LGTM, except there's apparently a merge conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants