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

Python 3.6 f-strings break notebook syntax highlighting #2037

Closed
bede opened this Issue Jan 11, 2017 · 16 comments

Comments

6 participants
@bede

bede commented Jan 11, 2017

Using Notebook v4.3.1, use of f-strings (PEP 498, Python 3.6) breaks syntax highlighting for the remaining code in a given cell.

Is Jupyter curently unable to handle this syntax? I would imagine that this issue would have been raised by others were this the case.

Using OS X, brewed Python 3.6.0, Firefox 50.

$ python3 --version
Python 3.6.0
$ jupyter --version
4.2.1
$ jupyter-notebook --version
4.3.1
$ sw_vers
ProductName:	Mac OS X
ProductVersion:	10.12.2
BuildVersion:	16C68

@bede bede changed the title from Python 3.6 f-strings break Notebook syntax highlighting to Python 3.6 f-strings break notebook syntax highlighting Jan 11, 2017

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 12, 2017

Member

It's an issue with Codemirror, the editor component. It has been fixed (codemirror/CodeMirror#4182), but we had to stick with an old version of Codemirror to work around an issue with Safari (#2004).

@gnestor @minrk what's the plan for getting back to a new version of Codemirror? Do we know if 5.22.1 fixes it?

Member

takluyver commented Jan 12, 2017

It's an issue with Codemirror, the editor component. It has been fixed (codemirror/CodeMirror#4182), but we had to stick with an old version of Codemirror to work around an issue with Safari (#2004).

@gnestor @minrk what's the plan for getting back to a new version of Codemirror? Do we know if 5.22.1 fixes it?

@bede

This comment has been minimized.

Show comment
Hide comment
@bede

bede Jan 12, 2017

Thanks, at least the fix is trivial.

Could we at least patch the existing Codemirror version? F-strings are the big ticket item in Python 3.6 after all.

bede commented Jan 12, 2017

Thanks, at least the fix is trivial.

Could we at least patch the existing Codemirror version? F-strings are the big ticket item in Python 3.6 after all.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 12, 2017

Member

The JS files are built into one big concatenated file. We don't have a good way to carry patches against upstream JS projects, as far as I know.

Member

takluyver commented Jan 12, 2017

The JS files are built into one big concatenated file. We don't have a good way to carry patches against upstream JS projects, as far as I know.

@bede

This comment has been minimized.

Show comment
Hide comment
@bede

bede Jan 12, 2017

Ah that's a pity, but understandable @takluyver. I've just grep'ed all js files in site-packages for rbu (safe from minification since it's a string?) and can't find this concatenated JS you mention... For the sake of myself and others patching it, please could tell me its path?

bede commented Jan 12, 2017

Ah that's a pity, but understandable @takluyver. I've just grep'ed all js files in site-packages for rbu (safe from minification since it's a string?) and can't find this concatenated JS you mention... For the sake of myself and others patching it, please could tell me its path?

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 12, 2017

Member

Should be notebook/static/notebook/js/main.min.js, I think

Member

takluyver commented Jan 12, 2017

Should be notebook/static/notebook/js/main.min.js, I think

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 13, 2017

Member

@gnestor the CodeMirror issue was closed, I think due to a comment that the bug wasn't reproducible outside the notebook. Does that mean it's due to our CSS somewhere?

Member

minrk commented Jan 13, 2017

@gnestor the CodeMirror issue was closed, I think due to a comment that the bug wasn't reproducible outside the notebook. Does that mean it's due to our CSS somewhere?

@bede

This comment has been minimized.

Show comment
Hide comment
@bede

bede Jan 13, 2017

Patch

Temporary solution tested on notebook version 4.3.1

Two patterns inside site-packages/notebook/static/notebook/js/main.min.js need replacing: [rub] with [rubf] and "rub" with "rubf"

With sed:

sed -i 's/'"rub"'/'"rubf"'/g' -i 's/\[rub\]/\[rubf\]/g' /usr/local/lib/python3.6/site-packages/notebook/static/notebook/js/main.min.js

bede commented Jan 13, 2017

Patch

Temporary solution tested on notebook version 4.3.1

Two patterns inside site-packages/notebook/static/notebook/js/main.min.js need replacing: [rub] with [rubf] and "rub" with "rubf"

With sed:

sed -i 's/'"rub"'/'"rubf"'/g' -i 's/\[rub\]/\[rubf\]/g' /usr/local/lib/python3.6/site-packages/notebook/static/notebook/js/main.min.js
@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Jan 13, 2017

Contributor

@minrk @takluyver I just tested 5.22.2 in Safari and the same issue as before. @jasongrout fixed this in lab by updating the CSS. I tried changing .CodeMirror-lines from padding: 0.4em to padding: 5px but no luck...

@bede I can't find a[rub] or "rub" in the source...

Contributor

gnestor commented Jan 13, 2017

@minrk @takluyver I just tested 5.22.2 in Safari and the same issue as before. @jasongrout fixed this in lab by updating the CSS. I tried changing .CodeMirror-lines from padding: 0.4em to padding: 5px but no luck...

@bede I can't find a[rub] or "rub" in the source...

@bede

This comment has been minimized.

Show comment
Hide comment
@bede

bede commented Jan 14, 2017

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jan 19, 2017

Member

I've marked this as 5.0 - I'd really like to get it fixed soon, because syntax highlighting is currently broken with the new Python 3.6 strings.

Are we aware of any fix for the Safari issue in CodeMirror that would let us update it again? If not, can we work out a way to use a patched CodeMirror?

Member

takluyver commented Jan 19, 2017

I've marked this as 5.0 - I'd really like to get it fixed soon, because syntax highlighting is currently broken with the new Python 3.6 strings.

Are we aware of any fix for the Safari issue in CodeMirror that would let us update it again? If not, can we work out a way to use a patched CodeMirror?

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Jan 20, 2017

Contributor

@takluyver Agreed! I've spent some time investigating the Safari issue and what I've concluded is that it was introduced in CodeMirror 5.17. To quote from codemirror/CodeMirror#4454 (comment):

Here is a list commits included in 5.17: https://github.com/codemirror/CodeMirror/commits/master?after=Y3Vyc29yOjiTKo4zLKAvc9OV%2BJnP36dpIuEyKzIwOQ%3D%3D

I have a feeling that this commit might be the culprit although I haven't been able to resolve this by simply reversing the changes in this commit: codemirror/CodeMirror@2c913e5

I will keep trying to isolate the issue. If it's not resolved by 5.0 release time, let's try patching CodeMirror.

Contributor

gnestor commented Jan 20, 2017

@takluyver Agreed! I've spent some time investigating the Safari issue and what I've concluded is that it was introduced in CodeMirror 5.17. To quote from codemirror/CodeMirror#4454 (comment):

Here is a list commits included in 5.17: https://github.com/codemirror/CodeMirror/commits/master?after=Y3Vyc29yOjiTKo4zLKAvc9OV%2BJnP36dpIuEyKzIwOQ%3D%3D

I have a feeling that this commit might be the culprit although I haven't been able to resolve this by simply reversing the changes in this commit: codemirror/CodeMirror@2c913e5

I will keep trying to isolate the issue. If it's not resolved by 5.0 release time, let's try patching CodeMirror.

@oldgeeksguide

This comment has been minimized.

Show comment
Hide comment
@oldgeeksguide

oldgeeksguide Jan 21, 2017

Thanks for the detailed info on the patch!

oldgeeksguide commented Jan 21, 2017

Thanks for the detailed info on the patch!

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Jan 21, 2017

Contributor

Ok, I've isolated this bug to this commit: codemirror/CodeMirror@e60f4b7

We'll see if we can fix it on the CM side... @takluyver Any idea why this commit is causing this behavior?

Contributor

gnestor commented Jan 21, 2017

Ok, I've isolated this bug to this commit: codemirror/CodeMirror@e60f4b7

We'll see if we can fix it on the CM side... @takluyver Any idea why this commit is causing this behavior?

@ruxi

This comment has been minimized.

Show comment
Hide comment
@ruxi

ruxi Jan 21, 2017

whats the recommended workaround for this?

ruxi commented Jan 21, 2017

whats the recommended workaround for this?

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Jan 21, 2017

Contributor

You can try @bede's patch: #2037 (comment)

Contributor

gnestor commented Jan 21, 2017

You can try @bede's patch: #2037 (comment)

@minrk minrk referenced this issue Jan 24, 2017

Merged

Patch CodeMirror 5.22 #2066

1 of 1 task complete

@gnestor gnestor modified the milestones: 4.3.2, 5.0 Jan 25, 2017

@minrk minrk closed this in #2066 Jan 25, 2017

@gnestor gnestor added this to Merged PRs in 4.3.2 Jan 30, 2017

@gnestor gnestor moved this from Merged PRs to Closed Issues in 4.3.2 Jan 30, 2017

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Feb 1, 2017

Contributor

Notebook 4.3.2 was just published 👍

Contributor

gnestor commented Feb 1, 2017

Notebook 4.3.2 was just published 👍

gnestor added a commit to gnestor/CodeMirror that referenced this issue Jun 23, 2017

Kilo59 added a commit to Kilo59/notebook that referenced this issue Aug 2, 2018

update codemirror component to 5.37
This should fix f string syntax highlighting 
jupyter#2037
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment