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

Rework special link parsing in the post-processing of markup #3354

Merged
merged 13 commits into from Feb 27, 2018

Conversation

7 participants
@thehowl
Copy link
Member

thehowl commented Jan 11, 2018

Fixes #3327. Adds the ability to link emails in markdown, both a@b.com, <a@b.com> and [hello](mailto:a@b.com), all of which previously didn't work. Also removed the autolink js library, as the link processing can be very easily done on the backend using a very simple blackfriday option.

Closes #3364. Closes #652.

In the new mechanism, the post processing is done by passing the input through a given set of functions, passed to a new postProcessCtx. This enables the ability to customise the post-processing features to enable, and thus also closes #2968. It also enables in the commit message many other features, such as linking, writing emails, and basically everything PostProcess does except for shortlinks.

Closes #3112 as well.

The only thing that seems to be needed is some unit tests, but this should be on its way for 1.5

@thehowl

This comment has been minimized.

Copy link
Member

thehowl commented Jan 11, 2018

Oh, I guess I kinda forgot about the unit tests... will need to rewrite them

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jan 11, 2018

But this will not render commit with just short sha clickable if I'm not wrong

@thehowl thehowl changed the title Email links in markdown; autolinking from backend [WIP] Email links in markdown; autolinking from backend Jan 11, 2018

@thehowl

This comment has been minimized.

Copy link
Member

thehowl commented Jan 11, 2018

But this will not render commit with just short sha clickable if I'm not wrong

It should still do AFAIK? What do you mean?

I've placed [WIP] on the header for the moment because I'm too tired now to iron out the remaining issue, but it's mostly done

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jan 12, 2018

I mean this (taken from tests):
You currently have:
<a href=\"http://localhost:3000/gogits/gogs/commit/b6dd6210eaebc915fd5be5579c58cce4da2e2579\" rel=\"nofollow\">http://localhost:3000/gogits/gogs/commit/b6dd6210eaebc915fd5be5579c58cce4da2e2579</a>
But it it needs to be (text between tag):
<a href=\"http://localhost:3000/gogits/gogs/commit/b6dd6210eaebc915fd5be5579c58cce4da2e2579\" rel=\"nofollow\">b6dd6210ea</a>

@thehowl thehowl referenced this pull request Jan 13, 2018

Closed

URLs in markdown are rendered with Javascript #3364

1 of 1 task complete

@thehowl thehowl changed the title [WIP] Email links in markdown; autolinking from backend [WIP] Rework special link parsing in the post-processing of markup Jan 13, 2018

@thehowl thehowl force-pushed the thehowl:fix-3327 branch 3 times, most recently from 8474447 to 930a5d6 Jan 13, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 13, 2018

Codecov Report

Merging #3354 into master will increase coverage by 0.35%.
The diff coverage is 89.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3354      +/-   ##
==========================================
+ Coverage   35.79%   36.15%   +0.35%     
==========================================
  Files         285      285              
  Lines       40868    40905      +37     
==========================================
+ Hits        14630    14790     +160     
+ Misses      24068    23943     -125     
- Partials     2170     2172       +2
Impacted Files Coverage Δ
modules/templates/helper.go 47.91% <37.5%> (-1.75%) ⬇️
modules/markup/markup.go 67.79% <40%> (+0.52%) ⬆️
modules/markup/markdown/markdown.go 66.35% <92.3%> (-2.35%) ⬇️
modules/markup/html.go 89.44% <92.43%> (+35.14%) ⬆️
models/repo_list.go 65.62% <0%> (-1.57%) ⬇️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 769ab1e...ab78354. Read the comment docs.

@thehowl thehowl changed the title [WIP] Rework special link parsing in the post-processing of markup Rework special link parsing in the post-processing of markup Jan 13, 2018

@thehowl

This comment has been minimized.

Copy link
Member

thehowl commented Jan 13, 2018

Since I got sick of the state of html.go, I rewrote it entirely making it use an HTML tree instead of a tokenizer + a bunch of sprintfs. I took the chance also to rewrite a bunch of stuff. It just so happened that because this enabled much more modularity than before, it also meant that implementing #2968 was pretty trivial.

image

Note that this does not enable markdown on commits.

Reviews welcome.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Jan 14, 2018

@thehowl Thanks for you great PR. I think we have serval kinds of render scenes. Markup File on the tree, Wiki, Issues(comments) and Notification Mail body. Currently, all support markdown syntax and Gitea Postprocesses. Markup File supports other syntax and Gitea Postprocesses. But there are some differences between markup files and wikis. So a set of Post processes could be a group, markup files and wikis could be different group.

@lunny lunny added this to the 1.x.x milestone Jan 14, 2018

@thehowl

This comment has been minimized.

Copy link
Member

thehowl commented Jan 14, 2018

You mean to make this into a different package?

@ypnos

This comment has been minimized.

Copy link

ypnos commented Jan 15, 2018

In Gitea 1.3.2, a URL like https://github.com/select2/select2/issues/4505#issuecomment-351836912 is not parsed correctly. The dash and the part after it are not seen as part of the URL.

Is this related to the work being done here?

@thehowl

This comment has been minimized.

Copy link
Member

thehowl commented Jan 15, 2018

@ypnos Slightly related, however the link you posted doesn't look like a case we need to handle seeing as it is an URL for a GitHub issue, and not the local instance. On the local instance, that is something we already handle; see this: https://try.gitea.io/Howl/aaa/issues/1

@ypnos

This comment has been minimized.

Copy link

ypnos commented Jan 15, 2018

This is just an example of an external link with a dash in it. So external URLs in the format http://… instead of […](http://…) are not part of this PR then?

@thehowl

This comment has been minimized.

Copy link
Member

thehowl commented Jan 15, 2018

External links in general are a part of this PR, though they were already supported before (using js, that is). They render as a simple link, e.g. https://google.com, instead of https://google.com. Is that what you mean?

@ypnos

This comment has been minimized.

Copy link

ypnos commented Jan 15, 2018

Great, so the current behavior is buggy and I wondered if you could check the new code for this. Please have a look at my example comment at https://try.gitea.io/Howl/aaa/issues/1

@thehowl

This comment has been minimized.

Copy link
Member

thehowl commented Jan 15, 2018

Interesting. Then yes, the bug still exists in my code. The way this could be solved is by, instead of using a regex, to use strings.FieldsFunc and find in the resulting array the strings beginning with http:// or https:// and for which url.Parse doesn't return an error. But that seems too much to implement in this PR - which is already pretty big. I suggest you to create an issue, linking to this conversation.

As a workaround, I recommend you use the standard markdown feature, which is that of wrapping the URL in angle brackets (e.g. <https://google.com>).

@ypnos

This comment has been minimized.

Copy link

ypnos commented Jan 15, 2018

Thank you.

After some of my own research on regular expressions for matching URLs I conclude that it is not worth the trouble for achieving more proper URL detection, hence I will not file another issue on this.

Btw. I also believe the more complicated approach with url.Parse would not be the end to it, for example it would most probably include a period at the end of the URL, which should in general be excluded.

@lafriks lafriks removed the status/wip label Feb 20, 2018

@lafriks lafriks modified the milestones: 1.x.x, 1.5.0 Feb 20, 2018

@tboerger tboerger added lgtm/need 1 and removed lgtm/need 2 labels Feb 20, 2018


"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
"github.com/stretchr/testify/assert"

This comment has been minimized.

@appleboy

appleboy Feb 21, 2018

Member

Missing empty line between local and external package.

See https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md#styleguide

This comment has been minimized.

@thehowl

thehowl Feb 22, 2018

Member

Addressed.

@thehowl thehowl force-pushed the thehowl:fix-3327 branch from 85c545d to 0dd2410 Feb 21, 2018

thehowl added some commits Jan 11, 2018

Make markdown tests work again
This is just a description to allow me to force push in order to restart
the drone build.

@thehowl thehowl force-pushed the thehowl:fix-3327 branch from 0dd2410 to 886d945 Feb 22, 2018

@tboerger tboerger added lgtm/done and removed lgtm/need 1 labels Feb 27, 2018

@lafriks lafriks merged commit 535445c into go-gitea:master Feb 27, 2018

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@thehowl thehowl referenced this pull request Mar 3, 2018

Closed

URLs in markdown are not auto linked #3446

2 of 7 tasks complete

@thehowl thehowl deleted the thehowl:fix-3327 branch Apr 22, 2018

@SagePtr SagePtr referenced this pull request Sep 13, 2018

Closed

Markdown tables are rendered bellow the footer #4212

1 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment