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.

Sign up for GitHub

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

Jump to bottom

Refactor renders #15175

Merged
merged 27 commits into from
Apr 19, 2021
Merged

Refactor renders #15175

merged 27 commits into from
Apr 19, 2021

Conversation

lunny
Copy link
Member

@lunny lunny commented Mar 27, 2021 •
edited
Loading

This PR refactor the Render system.

  • Rename Parser to Renderer because the previous name is confusing
  • Changed Render function signature of Renderer to accept input io.Reader and output io.Writer and return error
  • Reduce memory usage when possible.

@lunny lunny added pr/wip This PR is not ready for review type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Mar 27, 2021
@lunny lunny added this to the 1.15.0 milestone Mar 27, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 28, 2021
@lunny lunny force-pushed the lunny/refactor_render branch 4 times, most recently from 7407a70 to d99a6f7 Compare March 31, 2021 15:59
@lunny lunny changed the title WIP: Refactor renders Refactor renders Mar 31, 2021
6543
6543 reviewed Apr 1, 2021
View reviewed changes
modules/markup/csv/csv.go Outdated Show resolved Hide resolved
modules/markup/csv/csv.go Outdated Show resolved Hide resolved
modules/markup/csv/csv_test.go Show resolved Hide resolved
modules/markup/orgmode/orgmode.go Outdated Show resolved Hide resolved
routers/repo/view.go Outdated Show resolved Hide resolved
6543
6543 reviewed Apr 1, 2021
View reviewed changes
modules/markup/markdown/markdown.go
@@ -184,18 +185,26 @@ func actualRender(body []byte, urlPrefix string, metas map[string]string, wikiMa
_ = lw.CloseWithError(fmt.Errorf("%v", err))
}()

pc := NewGiteaParseContext(urlPrefix, metas, wikiMarkdown)
if err := converter.Convert(giteautil.NormalizeEOL(body), lw, parser.WithContext(pc)); err != nil {
// FIXME: Don't read all to memory, but goldmark doesn't support
Copy link
Member

Choose a reason for hiding this comment

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

exist an upstream issue for this?

Copy link
Member Author

@lunny lunny Apr 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think yes, I cannot find any function to allow io.Reader

Copy link
Member

Choose a reason for hiding this comment

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

yuin/goldmark#209

@lunny lunny force-pushed the lunny/refactor_render branch 2 times, most recently from 30a8387 to 8c88a22 Compare April 1, 2021 06:13
@lunny lunny removed the pr/wip This PR is not ready for review label Apr 1, 2021
@6543
Copy link
Member

6543 commented Apr 3, 2021

@lunny can you resolve conflicts .. made by (#15072)

@GiteaBot GiteaBot 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 Apr 3, 2021
@lunny lunny force-pushed the lunny/refactor_render branch 2 times, most recently from 7aa0736 to 4e0eda6 Compare April 17, 2021 09:18
@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@7417628). Click here to learn what that means.
The diff coverage is 55.93%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #15175   +/-   ##
=========================================
  Coverage          ?   43.82%           
=========================================
  Files             ?      678           
  Lines             ?    81730           
  Branches          ?        0           
=========================================
  Hits              ?    35815           
  Misses            ?    40082           
  Partials          ?     5833
Impacted Files Coverage Δ
modules/notification/mail/mail.go 33.67% <0.00%> (ø)
modules/setting/markup.go 1.58% <0.00%> (ø)
routers/org/home.go 61.00% <0.00%> (ø)
routers/repo/compare.go 40.93% <0.00%> (ø)
routers/repo/lfs.go 0.00% <0.00%> (ø)
routers/repo/milestone.go 0.00% <0.00%> (ø)
routers/user/profile.go 42.17% <0.00%> (ø)
modules/markup/external/external.go 2.00% <5.00%> (ø)
modules/markup/csv/csv.go 36.23% <22.22%> (ø)
routers/repo/release.go 23.19% <28.57%> (ø)
... and 20 more

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 7417628...1eeb587. Read the comment docs.

zeripath
zeripath reviewed Apr 17, 2021
View reviewed changes
modules/notification/mail/mail.go Outdated Show resolved Hide resolved
modules/notification/mail/mail.go Outdated Show resolved Hide resolved
modules/util/util.go Outdated Show resolved Hide resolved
routers/repo/wiki.go Show resolved Hide resolved
@lunny
Copy link
Member Author

lunny commented Apr 17, 2021 •
edited
Loading

Since we have switched to yuin/goldmark, the NormalEOF is unnecssary so I have removed the invoke about that.

@GiteaBot GiteaBot 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 Apr 19, 2021
@techknowlogick
Copy link
Member

🚀

@techknowlogick techknowlogick merged commit 9d99f6a into go-gitea:master Apr 19, 2021
@lunny lunny deleted the lunny/refactor_render branch April 19, 2021 23:28
6543 added a commit to 6543-forks/gitea that referenced this pull request Apr 22, 2021
@6543
Fix go-fuzz
df6a8bc 
followup of go-gitea#15175
@6543 6543 mentioned this pull request Apr 22, 2021
Merged
lunny pushed a commit that referenced this pull request Apr 23, 2021
@6543
Fix go-fuzz (#15596)
bee8ce3 
* Fix go-fuzz

followup of #15175

* simplify

* enhance
@lunny lunny mentioned this pull request Apr 28, 2021
Merged
@go-gitea go-gitea locked and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KN4CK3R KN4CK3R KN4CK3R left review comments

@6543 6543 6543 approved these changes

@zeripath zeripath zeripath approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
7 participants
@lunny @6543 @codecov-commenter @techknowlogick @KN4CK3R @zeripath @GiteaBot