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

Fixing issue preventing the correct read of images by full_html and reveal exporters. #3550

Merged
merged 2 commits into from Jul 6, 2013
Merged

Conversation

damianavila
Copy link
Member

Currently to see and images in the notebook inside a md cell you can use the prefix files/ to read the content of the directory where the ipynb lives. But, if you do a conversion using full_html or reveal exporters, the are not rendered ok because the previous version of this function erased /files/ instead of files/. With this fix, the html files produced by full_html and reveal exporters can read the images in a correct way.

…eveal exporters.

Currently to see and images in the notebook inside a md cell you can use the prefix files/ to read the content of the directory where the ipynb lives. But, if you do a conversion using full_html or reveal exporters, the are not rendered ok because the previous version of this function erased `/files/` instead of `files/`. With this fix, the html files produced by full_html and reveal exporters can read the images in a correct way.
@Carreau
Copy link
Member

Carreau commented Jul 5, 2013

actually, it would even be better if it removed only the first occurences in each url... it might be a little complicated though.

@minrk
Copy link
Member

minrk commented Jul 5, 2013

The logical fix you have made is right, but now the filter can be much too aggressive. This replacement should only ever apply inside urls, and only if the URL starts with files/.

@minrk
Copy link
Member

minrk commented Jul 5, 2013

Try this replacement:

    for attr in ('src', 'href'):
        for q in ('"', "'"):
            prefix = "{attr}={q}".format(attr=attr, q=q)
            text = text.replace(prefix + "files/", prefix)
    return text

And change the description to say that it only replaces URLs that start with files/.

I'm sure this can be done with a single re.sub call, but I don't want to figure out the right expression.

@minrk
Copy link
Member

minrk commented Jul 5, 2013

Actually, the regular expression is easy, if you want to just cherry-pick / merge a38edc4 into this PR.

@damianavila
Copy link
Member Author

I have problems cherry-picking / merging this one... maybe because I don't know enough from git...

My problem is fetching your code... I think is derived from my PR, but I could not find it any logical way to fetch it... any suggestion?

@minrk
Copy link
Member

minrk commented Jul 5, 2013

To add my changes to your repo:

git remote add minrk git://github.com/minrk/ipython.git
git fetch minrk

To add the changeset to your branch:

git cherry-pick a38edc4
# or
git merge minrk/pr/3550
# or, to be forceful if the others don't work for some reason:
git reset minrk/pr/3550 --hard

@damianavila
Copy link
Member Author

Thanks, I tried exactly what you explained... It has to be some typo... I will do it again and let you know

@damianavila
Copy link
Member Author

OK, done with the cherry-picking... cheking the history of my commands, it was a typo... grrr...

Anyways... it is working right here. I hope this could be merged soon.

@minrk
Copy link
Member

minrk commented Jul 6, 2013

Yup, 👍 for me, but I probably shouldn't be the last word, since I wrote the regex. @Carreau?

@Carreau
Copy link
Member

Carreau commented Jul 6, 2013

looks ok to me.

Carreau added a commit that referenced this pull request Jul 6, 2013
Fixing issue preventing the correct read of images by full_html and reveal exporters.

only strip `file/` at begining of URL.
@Carreau Carreau merged commit 2af80b7 into ipython:master Jul 6, 2013
@damianavila damianavila deleted the patch-1 branch July 6, 2013 18:31
@damianavila
Copy link
Member Author

Thanks!

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Fixing issue preventing the correct read of images by full_html and reveal exporters.

only strip `file/` at begining of URL.
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

3 participants