Add reStructedText support #2564

lunny · 2017-09-21T07:28:48Z

should be part of #374, see below screenshots.

codecov-io · 2017-09-21T07:47:32Z

Codecov Report

Merging #2564 into master will increase coverage by 0.04%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master    #2564      +/-   ##
==========================================
+ Coverage   27.33%   27.38%   +0.04%     
==========================================
  Files          86       87       +1     
  Lines       17137    17157      +20     
==========================================
+ Hits         4684     4698      +14     
- Misses      11775    11780       +5     
- Partials      678      679       +1

Impacted Files	Coverage Δ
modules/markup/orgmode/orgmode.go	`90.9% <ø> (ø)`	⬆️
...odules/markup/reStructuredText/reStructuredText.go	`70% <70%> (ø)`

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 fa6d7c7...6a4f0f8. Read the comment docs.

lunny · 2017-09-22T07:40:08Z

Tests added and rebased.

lafriks · 2017-09-22T13:18:04Z

LGTM

Morlinest · 2017-09-22T20:31:09Z

modules/markup/reStructuredText/reStructuredText_test.go

+)
+
+const AppURL = "http://localhost:3000/"
+const Repo = "gogits/gogs"


Why gogits/gogs?

Morlinest · 2017-09-22T20:37:01Z

modules/markup/reStructuredText/reStructuredText_test.go

+
+func TestRender_StandardLinks(t *testing.T) {
+	setting.AppURL = AppURL
+	setting.AppSubURL = AppSubURL


Shouldn't settings be set to previous value at the end of test to avoid affecting other tests? I think simple defer can be used for that.

Morlinest · 2017-09-22T20:48:06Z

modules/markup/reStructuredText/reStructuredText_test.go

+	setting.AppURL = AppURL
+	setting.AppSubURL = AppSubURL
+
+	test := func(input, expected string) {


Create test as separate function so you can use it for both tests.

Morlinest · 2017-09-22T20:51:10Z

modules/markup/reStructuredText/reStructuredText.go

+	return b.Bytes()
+}
+
+// RenderString reners reStructuredText string to HTML string


reners - typo? I think you can remove string from the end of comment.

Morlinest · 2017-09-22T21:00:37Z

modules/markup/reStructuredText/reStructuredText.go

+	return []string{".rst"}
+}
+
+// Render renders reStructuredText rawbytes to HTML


Is not rawbytes in comment redudant? And should be unified with RenderString comment.

Morlinest · 2017-09-22T21:09:03Z

modules/markup/reStructuredText/reStructuredText.go

+type Parser struct {
+}
+
+// Name implements markup.Parser


I think function comment should tell what that function is doing, not which interface it fulfils.

Morlinest · 2017-09-22T21:09:37Z

modules/markup/reStructuredText/reStructuredText.go

+	return "reStructuredText"
+}
+
+// Extensions implements markup.Parser


Same as for Name()

Morlinest · 2017-09-22T21:11:37Z

modules/markup/reStructuredText/reStructuredText_test.go

+
+func TestRender_Images(t *testing.T) {
+	setting.AppURL = AppURL
+	setting.AppSubURL = AppSubURL


Same as above.

lunny · 2017-09-23T06:15:22Z

@Morlinest all done.

ethantkoenig

IMO, using a third-party library that is "experimental/highly under development" (README) is not a good idea. Would it be worth finding a more stable library, or waiting until this one becomes more mature?

ethantkoenig · 2017-09-24T05:45:17Z

modules/markup/reStructuredText/reStructuredText.go

+	var b bytes.Buffer
+	w := bufio.NewWriter(&b)
+	p.ReStructuredText(bytes.NewReader(rawBytes), gorst.ToHTML(w))
+	w.Flush()


w.Flush() returns an error

lunny · 2017-09-24T05:51:57Z

@ethantkoenig Yes, but this is the most mature library I can find. Also, if #2570 could be merged, external tool could override internal library.

Morlinest · 2017-09-24T09:55:49Z

modules/markup/reStructuredText/reStructuredText_test.go

+	defer func() {
+		setting.AppURL = appURL
+		setting.AppSubURL = appSubURL
+	}()


I think you can simplify it like this:

defer func(appURL, appSubURL string) { setting.AppURL = appURL setting.AppSubURL = appSubURL }(setting.AppURL, setting.AppSubURL)

or to use same deffer for both tests:

func resetSetings(appURL, appSubURL string) { setting.AppURL = appURL setting.AppSubURL = appSubURL } ... defer resetSetings(setting.AppURL, setting.AppSubURL)

Now, when I look at whole change at once... Why you need to change settings? I see only setting.AppSubURL is used in test().

Morlinest · 2017-09-24T10:11:36Z

modules/markup/reStructuredText/reStructuredText_test.go

+const AppSubURL = AppURL + Repo + "/"
+
+func test(t *testing.T, input, expected string) {
+	buffer := RenderString(input, setting.AppSubURL, nil, false)


Use setting.AppSubURL as another test() argument. E.g. test(t *testing.T, input, expected, appSubURL string)

Morlinest · 2017-09-24T10:16:31Z

modules/markup/reStructuredText/reStructuredText_test.go

+
+const AppURL = "http://localhost:3000/"
+const Repo = "go-gitea/gitea"
+const AppSubURL = AppURL + Repo + "/"


If you don't need all constants (look at comment for defer), merge them together. Also I think you can use unexported version. E.g. const appSubURL = "http://localhost:3000/go-gitea/gitea/".

Morlinest · 2017-09-24T10:22:04Z

modules/markup/reStructuredText/reStructuredText_test.go

+		`<p><a href="`+lnk+`">WikiPage</a></p>`)*/
+}
+
+func TestRender_Images(t *testing.T) {


If you don' need to change settings (look at defer comment above), you can merge both tests together, remove setting variable changes and defer and use t.Run() (just create array of test cases) to run tests in parallel.

ethantkoenig

I still do not think that using an unstable library is a good idea, but I'll approve so that I don't block the PR.

lunny · 2017-09-25T05:12:13Z

@ethantkoenig I don't mind to put this PR later. If #2570 could be merged, it also resolved #374.

lunny · 2017-09-25T05:13:17Z

And maybe https://github.com/demizer/go-rst is a better choice.

ghost · 2018-07-21T18:13:02Z

@lunny go-rst is marked as ABANDONED. I think with #374 closed this could also be closed.

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Sep 21, 2017

lunny added this to the 1.3.0 milestone Sep 21, 2017

lunny added the pr/wip This PR is not ready for review label Sep 21, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 21, 2017

lunny mentioned this pull request Sep 21, 2017

Consider support for reST (reStructuredText) preview #2243

Closed

7 tasks

lunny force-pushed the lunny/reStructedText branch from 057ce9d to de1bdb8 Compare September 22, 2017 07:36

lunny removed the pr/wip This PR is not ready for review label Sep 22, 2017

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 22, 2017

Morlinest suggested changes Sep 22, 2017

View reviewed changes

lunny force-pushed the lunny/reStructedText branch from de1bdb8 to 249fec3 Compare September 23, 2017 06:14

ethantkoenig suggested changes Sep 24, 2017

View reviewed changes

lunny added 5 commits September 24, 2017 13:59

add reStructedText support and fix readme render bug

49362fc

add tests

7282b99

improve tests and fix comments

ebb63fc

fix tests

1e07fb0

handle the error on render reStructuredText

6a4f0f8

lunny force-pushed the lunny/reStructedText branch from f9e945b to 6a4f0f8 Compare September 24, 2017 05:59

Morlinest suggested changes Sep 24, 2017

View reviewed changes

ethantkoenig approved these changes Sep 25, 2017

View reviewed changes

lunny modified the milestones: 1.3.0, 1.x.x Sep 25, 2017

lunny added the pr/wip This PR is not ready for review label Sep 25, 2017

lunny added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Sep 25, 2017

lunny changed the title ~~Add reStructedText support and fix readme render bug~~ Add reStructedText support Nov 7, 2017

bkcsoft 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 Jul 21, 2018

lunny removed this from the 1.x.x milestone Jul 22, 2018

lunny closed this Jul 22, 2018

lunny deleted the lunny/reStructedText branch November 18, 2020 04:34

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reStructedText support #2564

Add reStructedText support #2564

lunny commented Sep 21, 2017

codecov-io commented Sep 21, 2017 •

edited

Loading

lunny commented Sep 22, 2017

lafriks commented Sep 22, 2017

Morlinest Sep 22, 2017

Morlinest Sep 22, 2017

Morlinest Sep 22, 2017

Morlinest Sep 22, 2017

Morlinest Sep 22, 2017

Morlinest Sep 22, 2017

Morlinest Sep 22, 2017

Morlinest Sep 22, 2017

lunny commented Sep 23, 2017

ethantkoenig left a comment •

edited

Loading

ethantkoenig Sep 24, 2017 •

edited

Loading

lunny Sep 24, 2017

lunny commented Sep 24, 2017

Morlinest Sep 24, 2017

Morlinest Sep 24, 2017

Morlinest Sep 24, 2017

Morlinest Sep 24, 2017

Morlinest Sep 24, 2017

ethantkoenig left a comment

lunny commented Sep 25, 2017

lunny commented Sep 25, 2017

ghost commented Jul 21, 2018

Add reStructedText support #2564

Add reStructedText support #2564

Conversation

lunny commented Sep 21, 2017

codecov-io commented Sep 21, 2017 • edited Loading

Codecov Report

lunny commented Sep 22, 2017

lafriks commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Sep 23, 2017

ethantkoenig left a comment • edited Loading

Choose a reason for hiding this comment

ethantkoenig Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig left a comment

Choose a reason for hiding this comment

lunny commented Sep 25, 2017

lunny commented Sep 25, 2017

ghost commented Jul 21, 2018

codecov-io commented Sep 21, 2017 •

edited

Loading

ethantkoenig left a comment •

edited

Loading

ethantkoenig Sep 24, 2017 •

edited

Loading