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

nbconvert: Fix sphinx preprocessor date format string for Windows #4085

Merged
merged 6 commits into from Sep 4, 2013

Conversation

jdfreder
Copy link
Member

This is a fix for a regression that was introduced in 32ea709.

@takluyver
Copy link
Member

There appears to be one other instance of %-d in the same file. Do you want to clean that up in this PR as well?

Connecting this to the previous issue #3719 and pull request #3720.

@ivanov
Copy link
Member

ivanov commented Aug 29, 2013

since we're talking about this again, does anyone else think it's weird that we're making our output uglier for most of our users, just to accommodate windows users?

In [5]: datetime.datetime(2012,1,1).strftime("%B %d, %Y")
Out[5]: 'January 01, 2012'

instead of not having a leading zero when we use %-d. Can we just code this in as a platform-specific ugliness, as opposed to an OS agnostic ugliness?

@minrk
Copy link
Member

minrk commented Sep 3, 2013

@jdfreder - plans to implement @ivanov's suggestion?

@jdfreder
Copy link
Member Author

jdfreder commented Sep 3, 2013

Must have flew under my radar, sure sounds like a good idea to me. Will post back when implemented (couple min to switch branches)

@jdfreder
Copy link
Member Author

jdfreder commented Sep 3, 2013

Ah, I see why this flew off my radar, in the latex template refact. I allow latex to use it's default date (which is the time of the document build). If users want to override it, they do so using templates (part of moving people away from setting config=true variables where templating can be used).

I'll go ahead and 'fix' this so it can be back ported. In the refactored templates the sphinx preprocessor is almost empty.

@jdfreder
Copy link
Member Author

jdfreder commented Sep 3, 2013

Should be good now

@takluyver
Copy link
Member

Are there other places we might need a formatted date? Is it worth pulling it out into some default_date_format variable? Otherwise, 👍

@jdfreder
Copy link
Member Author

jdfreder commented Sep 3, 2013

@takluyver https://github.com/ipython/ipython/blob/master/IPython/nbconvert/exporters/exporter.py#L286 , maybe it would be useful to have this behaviour here too

@takluyver
Copy link
Member

OK. I wonder if this belongs somewhere in IPython.utils - maybe IPython.utils.text. Or is there a logical place for it in nbconvert?

@jdfreder
Copy link
Member Author

jdfreder commented Sep 3, 2013

^ nothing better than IPython.utils. In nbconvert it could be put in a utils.strings module. I guess it depends on if you think it will be used outside of nbconvert?

@takluyver
Copy link
Member

I can imagine using it in something like the notebook HTML interface.

@jdfreder
Copy link
Member Author

jdfreder commented Sep 4, 2013

Tell me if that looks good

@@ -18,6 +18,7 @@

# Stdlib imports
import os.path
import sys
Copy link
Member

Choose a reason for hiding this comment

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

I think this import is now superfluous.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not on my machine, still required - py3.3

@takluyver
Copy link
Member

I'm not entirely convinced about making this a function - we might want one or more config attributes later to overwrite the default format. What about just doing a default_date_format variable at the module level? That does mean we can't use the {} trick on Windows, but I think the added flexibility is more valuable.

@jdfreder
Copy link
Member Author

jdfreder commented Sep 4, 2013

does mean we can't

Either way we need logic to discern between win32 and else, so I don't see what you mean ... unless you're suggesting ignoring @ivanov 's suggestion?

@takluyver
Copy link
Member

I mean, something like this:

if sys.platform == 'win32':
    default_date_format = "%B %d, %Y"
else:
    default_date_format = "%B %-d, %Y"

And then using e.g. modified_date.strftime(default_date_format).

@jdfreder
Copy link
Member Author

jdfreder commented Sep 4, 2013

And then using e.g. modified_date.strftime(default_date_format).

I added a new region to the text.py file since there aren't any other public variables in that module.

@jdfreder
Copy link
Member Author

jdfreder commented Sep 4, 2013

there aren't any other public variables in that module.

I think this is the only reason (and the {0} trick) to use a function

@takluyver
Copy link
Member

Thanks, that's what I was thinking of. I don't think there's any special reason to avoid using public variables - after all, a function is just a public variable pointing to a callable object.

@jdfreder
Copy link
Member Author

jdfreder commented Sep 4, 2013

True 😃

@minrk
Copy link
Member

minrk commented Sep 4, 2013

ready to merge, then?

@jdfreder
Copy link
Member Author

jdfreder commented Sep 4, 2013

I am, @takluyver ?

takluyver added a commit that referenced this pull request Sep 4, 2013
nbconvert: Fix sphinx preprocessor date format string for Windows
@takluyver takluyver merged commit 5e4d9f3 into ipython:master Sep 4, 2013
@jdfreder jdfreder deleted the fix_sphinx_data_win_again branch March 10, 2014 18:42
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
nbconvert: Fix sphinx preprocessor date format string for Windows
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.

None yet

4 participants