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

Remove final newline from rendered code #18076

Closed
wants to merge 3 commits into from
Closed

Remove final newline from rendered code #18076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96238e9
Remove final newline from rendered code
silverwind May 20, 2022
d5cd64a
add setting ui.HIDE_FINAL_NEWLINE
silverwind May 20, 2022
0c8605c
grammar
silverwind May 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions custom/conf/app.example.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1134,6 +1134,9 @@ PATH =
;;
;; Whether to enable a Service Worker to cache frontend assets
;USE_SERVICE_WORKER = false
;;
;; Whether to hide a final newline in rendered code
;HIDE_FINAL_NEWLINE = true

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Expand Down
1 change: 1 addition & 0 deletions docs/content/doc/advanced/config-cheat-sheet.en-us.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ The following configuration set `Content-Type: application/vnd.android.package-a
- `DEFAULT_SHOW_FULL_NAME`: **false**: Whether the full name of the users should be shown where possible. If the full name isn't set, the username will be used.
- `SEARCH_REPO_DESCRIPTION`: **true**: Whether to search within description at repository search on explore page.
- `USE_SERVICE_WORKER`: **false**: Whether to enable a Service Worker to cache frontend assets.
- `HIDE_FINAL_NEWLINE`: **true**: Whether to hide a final newline in rendered code

### UI - Admin (`ui.admin`)

Expand Down
5 changes: 4 additions & 1 deletion modules/highlight/highlight.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ func CodeFromLexer(lexer chroma.Lexer, code string) string {
}

htmlw.Flush()

// Chroma will add newlines for certain lexers in order to highlight them properly
// Once highlighted, strip them here so they don't cause copy/paste trouble in HTML output
return strings.TrimSuffix(htmlbuf.String(), "\n")
Expand Down Expand Up @@ -190,8 +191,9 @@ func File(numLines int, fileName, language string, code []byte) []string {
}

htmlw.Flush()

finalNewLine := false
if len(code) > 0 {
if !setting.UI.HideFinalNewline && len(code) > 0 {
finalNewLine = code[len(code)-1] == '\n'
}
Comment on lines 195 to 198
Copy link
Member

Choose a reason for hiding this comment

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

a) these four lines can be shortened to a single line because we are only using booleans here (GitHub doesn't let me make a suggestion for some reason...)
b) I think a comment would be nice, that method is already large enough and could use some structuring so that you don't become confused on what has happened already, and what has yet to happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would that single line be? It's not obvious to me

Copy link
Member

Choose a reason for hiding this comment

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

finalNewLine := !setting.UI.HideFinalNewline && len(code) > 0 && code[len(code)-1] == '\n'


Expand All @@ -208,6 +210,7 @@ func File(numLines int, fileName, language string, code []byte) []string {
content = strings.TrimPrefix(content, `</span>`)
m = append(m, content)
}

if finalNewLine {
m = append(m, "<span class=\"w\">\n</span>")
}
Expand Down
2 changes: 0 additions & 2 deletions modules/highlight/highlight_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@ steps:
`</span></span><span class="line"><span class="cl"><span class="w"> </span>- <span class="l">go build -v</span>`,
`</span></span><span class="line"><span class="cl"><span class="w"> </span>- <span class="l">go test -v -race -coverprofile=coverage.txt -covermode=atomic</span><span class="w">
</span></span></span>`,
`<span class="w">
</span>`,
Comment on lines -56 to -57
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add a test where we test for the behavior if there is a newline, and HIDE_FINAL_NEWLINE = false?

Copy link
Member Author

@silverwind silverwind May 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure whether these kind of tests should be extended because the output of go test is a major pain to work with in case of string arrays. It took me like 10 minutes to figure out what to change to get the test to pass.

Maybe if it were to test a single multiline detented string, it would be easier to work with. I can check later what can be done to make these tests easier to work with.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some test improvments in #19798, so I will revisit here after that one is merged.

},
},
{
Expand Down
2 changes: 2 additions & 0 deletions modules/setting/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ var (
CustomEmojisMap map[string]string `ini:"-"`
SearchRepoDescription bool
UseServiceWorker bool
HideFinalNewline bool

Notification struct {
MinTimeout time.Duration
Expand Down Expand Up @@ -1069,6 +1070,7 @@ func loadFromConf(allowEmpty bool, extraConfig string) {
UI.DefaultShowFullName = Cfg.Section("ui").Key("DEFAULT_SHOW_FULL_NAME").MustBool(false)
UI.SearchRepoDescription = Cfg.Section("ui").Key("SEARCH_REPO_DESCRIPTION").MustBool(true)
UI.UseServiceWorker = Cfg.Section("ui").Key("USE_SERVICE_WORKER").MustBool(false)
UI.HideFinalNewline = Cfg.Section("ui").Key("HIDE_FINAL_NEWLINE").MustBool(true)

HasRobotsTxt, err = util.IsFile(path.Join(CustomPath, "robots.txt"))
if err != nil {
Expand Down