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

A few typos corrected, and generic formatting (PEP 8) #10

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

Conversation

tobiasherp
Copy link

This doesn't contain any functional changes; I corrected a few typos (pathes -> patches), and I did some generic format corrections.

Before using pep8ify -wn, I removed the trailing spaces (which would have been removed by pep8ify as well) to make the process more transparent.

Pep8ify has currently a bug (it inserts a space between a comma and the closing ) of a tuple, so I corrected this in another step.

utils module includes:
  safe_decode function (and factory make_safe_decoder);
  add_info/get_info, to store and access cumulated information;
  helpers for tarball download
The WriterSection uses add_info to tell the transmogrifier about the
ExportContext.
The ValueError raised by lxml.etree is not very helpful; in my case, the
problem was a vertical tab (0x0b, one of string.whitespace).
This change makes the inappropriate value visible, at least to observers of a
foreground session.
if safe_decode is given, use it for *all* strings, including unicode;
it might serve additional purposes, e.g. removal of vertical tabs etc.
(which can cause lxml to crash)
This enables sections to specify a whitelist of properties they are
interested in.

By default, the Helper class skips the i18n_domain property.
It is now enabled to use a whitelist *or* a blacklist of property names;
unless given a whitelist, 'i18n_domain' will still be skipped.

A method _skip_this is generated by the factory function make_skipfunc.
When created with verbose=True, the skip function will write a line to
stdout whenever called.
... which was never meant for the master branch
Two changes:
- Generally feed the xml text to the safe_decode function which will try
  utf-8 first, then latin-1.  Another safe_decode function could be
  generated with other supported encodings, and it could be
  configurable, but this was sufficient to make my export succeed.
- If this is not sufficient, and an ExpatError still occurs, try to be
  as helpful as possible printing information to stdout.
Enable the calling code to inform the user about the location of the
written directory
try:
doc = minidom.parseString(xml)
except ExpatError as e:
print '-->' * 26
Copy link
Member

Choose a reason for hiding this comment

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

Don't use print. You wil only see it, if instance runs in foreground and it hurts if running doctests. See http://blog.redturtle.it/2010/02/15/do-not-use-print.-again-plone_log-is-your-friend

Copy link
Author

Choose a reason for hiding this comment

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

Ok, print is not the ultimate solution, and it could be improved. It won't hurt doctests in this case because it is done for ExpatErrors only (unless there are doctests for such errors).

I had a very hard time to find out where the error occurred which made my export fail; and I re-raise the exception. The default output of ExpatErrors is not sufficiently helpful. Having useful information (which enabled me to tweak my export to make it work) in foreground sessions only is better than having it not at all.

@tobiasherp
Copy link
Author

Oops, right. Github apparently continuously updates my pull request and added commits which I didn't intend to include (this applies to print statements and the safe_decode function).

My original pull request included just what the title says; perhaps we can start with the commits up to July 22nd. I'm pretty sure they don't break anything; IIRC, the travis tests failed before as well.

@gforcada
Copy link
Member

@tobiasherp friendly reminder that this PR is still open :)

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