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 URL handling in the whole markdown module, improve test coverage (fix #997) #1027

Merged
merged 1 commit into from
Feb 24, 2017
Merged

Fix URL handling in the whole markdown module, improve test coverage (fix #997) #1027

merged 1 commit into from
Feb 24, 2017

Conversation

andrew-boyarshin
Copy link
Contributor

@andrew-boyarshin andrew-boyarshin commented Feb 23, 2017

As it is in the title.

Plus, links to issues or PRs in external repositories will no longer be rewritten to current repo.

Fixes issue #997. Merging this PR will increase coverage to 32.4% from 31.8%.

@@ -9,4 +9,5 @@ type MarkdownOption struct {
Text string
Mode string
Context string
Wiki string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Public Gitea API changed, optional Wiki parameter added.

Copy link
Member

Choose a reason for hiding this comment

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

But you have to send a PR to SDK project at first before this can be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lunny done.

@@ -74,7 +75,7 @@ func TestAPI_RenderGFM(t *testing.T) {
<ul>
<li><a href="` + AppSubURL + `wiki/Links" rel="nofollow">Links, Language bindings, Engine bindings</a></li>
<li><a href="` + AppSubURL + `wiki/Tips" rel="nofollow">Tips</a></li>
<li>Bezier widget (by <a href="` + AppURL + `r-lyeh" rel="nofollow">@r-lyeh</a>)<a href="` + AppSubURL + `issues/786" rel="nofollow">#786</a></li>
<li>Bezier widget (by <a href="` + AppURL + `r-lyeh" rel="nofollow">@r-lyeh</a>)<a href="https://github.com/ocornut/imgui/issues/786" rel="nofollow">#786</a></li>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, link hosts are no longer rewritten. No more dead links.

res += "/"
}
}
cwdIndex := strings.Index(res, "/./")
Copy link
Contributor Author

@andrew-boyarshin andrew-boyarshin Feb 23, 2017

Choose a reason for hiding this comment

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

URLJoin now correctly handles . and .. path elements.

@@ -92,10 +92,10 @@ var (
ShortLinkPattern = regexp.MustCompile(`(\[\[.*\]\]\w*)`)

// AnySHA1Pattern allows to split url containing SHA into parts
AnySHA1Pattern = regexp.MustCompile(`http\S+//(\S+)/(\S+)/(\S+)/(\S+)/([0-9a-f]{40})(?:/?([^#\s]+)?(?:#(\S+))?)?`)
AnySHA1Pattern = regexp.MustCompile(`(http\S*)://(\S+)/(\S+)/(\S+)/(\S+)/([0-9a-f]{40})(?:/?([^#\s]+)?(?:#(\S+))?)?`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Protocol is important now.

@lunny lunny added this to the 1.1.0 milestone Feb 23, 2017
@lunny lunny added the type/bug label Feb 23, 2017
@@ -9,4 +9,5 @@ type MarkdownOption struct {
Text string
Mode string
Context string
Wiki string
Copy link
Member

Choose a reason for hiding this comment

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

But you have to send a PR to SDK project at first before this can be merged.

@lunny
Copy link
Member

lunny commented Feb 24, 2017

LGTM except the Wiki type should be bool

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 24, 2017
Amended with string to bool change in API SDK.

Signed-off-by: Andrew Boyarshin <andrew.boyarshin@gmail.com>
@andrew-boyarshin
Copy link
Contributor Author

andrew-boyarshin commented Feb 24, 2017

In case someone hasn't noticed, I force-pushed amended commits to both gitea and gitea-sdk repos with string -> bool change. Review for the final LGTM?

@appleboy
Copy link
Member

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 24, 2017
@andrew-boyarshin
Copy link
Contributor Author

Pending merge.

@lunny lunny merged commit 0602a44 into go-gitea:master Feb 24, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants