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

Restructure markup & markdown to prepare for multiple markup language… #2411

Merged
merged 6 commits into from
Sep 16, 2017
Merged

Restructure markup & markdown to prepare for multiple markup language… #2411

merged 6 commits into from
Sep 16, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Aug 28, 2017

As title.

@lunny lunny added type/enhancement An improvement of existing functionality type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Aug 28, 2017
@lunny lunny added this to the 1.3.0 milestone Aug 28, 2017
bkcsoft
bkcsoft reviewed Aug 28, 2017
View reviewed changes
Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

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

Few comments, otherwise LGTM

modules/markup/html.go Outdated

var (
// NOTE: All below regex matching do not perform any extra validation.
// Thus a link is produced even if the user does not exist, the issue does not exist, the commit does not exist, etc.
Copy link
Member

@bkcsoft bkcsoft Aug 28, 2017 •
edited by lafriks
Loading

Choose a reason for hiding this comment

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

even if the linked entity does not exist would suffice

modules/markup/html.go Outdated
var (
// NOTE: All below regex matching do not perform any extra validation.
// Thus a link is produced even if the user does not exist, the issue does not exist, the commit does not exist, etc.
// While fast, this is also incorrect and lead to false positives.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should document this somewhere else than as a code comment. Or add a TODO: fix invalid linking issue or similar.

One thought is having a markdown page in our docs to point our differences in markdown rendering between Gitea, Github, etc. WDYT?

@lafriks
Copy link
Member

lafriks commented Aug 28, 2017

Tests seems to fail

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 28, 2017
@lunny
Copy link
Member Author

lunny commented Aug 28, 2017

@lafriks Yes, I know. I will fix it later.

@lunny lunny force-pushed the lunny/mark_up_structure branch 2 times, most recently from fbc8188 to 8a1f5c4 Compare September 5, 2017 14:21
@codecov-io
Copy link

codecov-io commented Sep 15, 2017 •
edited
Loading

Codecov Report

Merging #2411 into master will decrease coverage by 0.72%.
The diff coverage is 46.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2411      +/-   ##
==========================================
- Coverage   27.59%   26.86%   -0.73%     
==========================================
  Files          83       84       +1     
  Lines       16928    16936       +8     
==========================================
- Hits         4671     4550     -121     
- Misses      11581    11712     +131     
+ Partials      676      674       -2
Impacted Files Coverage Δ
modules/markup/sanitizer.go 82.6% <ø> (ø)
models/issue_mail.go 5.97% <0%> (ø) ⬆️
models/mail.go 0% <0%> (ø) ⬆️
routers/api/v1/misc/markdown.go 78% <100%> (ø) ⬆️
models/issue_comment.go 31.51% <100%> (ø) ⬆️
modules/markup/markup.go 45.45% <39.47%> (+20.45%) ⬆️
modules/markup/html.go 46.24% <46.24%> (ø)
modules/markdown/markdown.go 66.95% <73.33%> (-12.29%) ⬇️
models/repo.go 13.53% <75%> (ø) ⬆️
... and 1 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 911ca02...7e6f9e9. Read the comment docs.

@lunny
Copy link
Member Author

lunny commented Sep 15, 2017

@bkcsoft done. Since this is a refactor PR, I will not change a lot but splitting codes into markup and markdown. Codes on markup will be public by future other doc types. Codes on markdown should be the special content for markdown.

@lafriks
Copy link
Member

lafriks commented Sep 15, 2017

LGTM

@tboerger 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 15, 2017
@lafriks
Copy link
Member

lafriks commented Sep 15, 2017

@bkcsoft your LG-TM did not count as it is in review comments

@bkcsoft
Copy link
Member

bkcsoft commented Sep 16, 2017

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 Sep 16, 2017
@lafriks lafriks merged commit 52e11b2 into go-gitea:master Sep 16, 2017
@lunny lunny deleted the lunny/mark_up_structure branch September 17, 2017 01:06
@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.
Reviewers

@bkcsoft bkcsoft bkcsoft left review comments

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

Successfully merging this pull request may close these issues.
5 participants
@lunny @lafriks @codecov-io @bkcsoft @tboerger