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

Fix autolink #8496

Merged
merged 11 commits into from May 30, 2020
Merged

Fix autolink #8496

merged 11 commits into from May 30, 2020

Conversation

gcbeltramini
Copy link
Contributor

@gcbeltramini gcbeltramini commented May 29, 2020

References

Issue #8376

Code changes

I suggest proposal 2, since it's the current behavior of Jupyter.

User-facing changes

Tested with:

print('Foo http://127.0.0.1/test?query=string&param=42 bar')
print('Foo <https://jupyter.org/> bar')
print('Foo https://jupyter.org/> bar')
print('Foo <https://jupyter.org/ bar')
print('Foo https://jupyter.org/ bar')

Proposal 1

Screen Shot 2020-05-29 at 00 28 53

  • http://127.0.0.1/test?query=string&param=42 is rendered correctly
  • But when a URL is followed by >, it includes > in the link, and an extra character ; is appended. The function autolink seems to be working as expected (see the alert message), but I couldn't understand why this was happening

I tried a second approach.

Proposal 2

Screen Shot 2020-05-29 at 00 33 13

This is exactly the same behavior as in Jupyter notebook.

  • http://127.0.0.1/test?query=string&param=42 is rendered correctly
  • a URL preceded by < is not rendered
  • if there is a character > in the end of the URL, it is erroneously included in the link

Backwards-incompatible changes

Some links were not rendered correctly. This is a fix.

@jupyterlab-dev-mode
Copy link

jupyterlab-dev-mode bot commented May 29, 2020

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor

jasongrout commented May 29, 2020

Thanks for exploring this deeply. IIRC, #8075 initially implemented this feature using the code from the classic notebook, but it had problems, and so #8240 improved it. Since this is basically reverting #8240, can you comment on what we are missing from the improvements #8240 introduced?

@gcbeltramini
Copy link
Contributor Author

gcbeltramini commented May 30, 2020

Thanks for exploring this deeply. IIRC, #8075 initially implemented this feature using the code from the classic notebook, but it had problems, and so #8240 improved it. Since this is basically reverting #8240, can you comment on what we are missing from the improvements #8240 introduced?

I added more tests to compare the regular expressions:

  • Add back autolink support #8075: doesn't recognize the URL when it's followed by any of these characters / # < >
  • Improve autolink #8240: after a minor improvement in the regex (this commit), doesn't recognize the URL when it's followed by any of these characters < >
  • classic jupyter: doesn't recognize the URL when it's followed by any of these characters ' " ( ) { } [ ] < >, and if followed by , : ; . ! ?, these characters are included in the rendered link.

Now, after looking at the examples, I suggest to use the regex from #8240 (with the minor improvements) and, instead of creating the HTML element, building the string explicitly (this avoids the conversion from & to &amp;, which changes the URL to an incorrect link).

Edit: The problem described below is fixed in the next commits. Now <https://jupyter.org> becomes <https://jupyter.org>, and >https://jupyter.org< becomes >https://jupyter.org<.

But there is a remaining problem (which is the original issue) that I could not fix (the screenshots are in the PR description):

The tests are commented out in the tests. Do you have any idea how to fix it? I included <> in the regex, and the function is working fine. The problem seems to be somewhere else.

Copy link
Member

@blink1073 blink1073 left a comment

Nice work, thanks!

@blink1073 blink1073 merged commit 1969129 into jupyterlab:master May 30, 2020
38 of 39 checks passed
@blink1073 blink1073 added this to the 2.2 milestone May 30, 2020
@blink1073
Copy link
Member

blink1073 commented May 30, 2020

Congratulations on your first contribution to JupyterLab @gcbeltramini!

saulshanabrook pushed a commit that referenced this issue Jun 25, 2020
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:rendermime status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants