Show new lines, tabs, whitespaces#211
Conversation
|
Awesome @smacker ! |
dpordomingo
left a comment
dpordomingo
left a comment
There was a problem hiding this comment.
thanks!
storing the user preference in the local storage was a great idea, and it works like a charm!
I'd only require some cosmetic and organizational changes.
| filePair.Right.Content, | ||
| preprocessors..., | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
This one is duplicated some lines below https://github.com/src-d/code-annotation/pull/211/files#diff-fcf9e14a0be9f2c9ac7cd962eaa11d10R55
server/service/diff.go
Outdated
| contentB = p(contentB) | ||
| } | ||
|
|
||
| diffString, err := d.generate(nameA, nameB, contentA, contentB) |
There was a problem hiding this comment.
this line could be replaced by
retun d.generate(nameA, nameB, contentA, contentB)And then the code below that would be unnecessary
server/service/diff.go
Outdated
|
|
||
| // DiffReplaceInvisible preprocessor function that replace invisible character with visible onces | ||
| func DiffReplaceInvisible(content string) string { | ||
| lines := strings.Split(content, "\n") |
There was a problem hiding this comment.
Do we need to split → replace → join?
Could we replace it by...
func ReplaceInvisible(content string) string {
line := strings.Replace(content, " ", "·", -1)
line = strings.Replace(line, "\t", "→", -1)
line = strings.Replace(line, "\r", "^M", -1)
line = strings.Replace(line, "\n", "↵\n", -1)
return line
}
server/service/diff.go
Outdated
| } | ||
|
|
||
| // DiffReplaceInvisible preprocessor function that replace invisible character with visible onces | ||
| func DiffReplaceInvisible(content string) string { |
There was a problem hiding this comment.
Diff should not be part of the function name
server/service/diff_test.go
Outdated
|
|
||
| assert.Equal( | ||
| service.DiffReplaceInvisible(diffStr), | ||
| "---·a.txt^M↵\n+++·b.txt^M↵\nline^M↵\n····tabbed·line^M↵\n····spaced·line^M↵\n", |
There was a problem hiding this comment.
since Diff should not be part of the ReplaceInvisible function name, it should be applied everywhere, what means that the test data should not be a diff file (because then, it looks a different thing)
server/service/diff_test.go
Outdated
|
|
||
| assert.Equal( | ||
| service.DiffReplaceInvisible(diffStr), | ||
| "---·a.txt^M↵\n+++·b.txt^M↵\nline^M↵\n····tabbed·line^M↵\n····spaced·line^M↵\n", |
There was a problem hiding this comment.
If the test input data is loaded from a file, the expected data should be also loaded from a file.
imo, both: input and expected should be defined inside of the test case function instead of inside an external file (they're too simple to be hidden inside a file), what is also more explicit and clear.
input := "line\r\n".
"\ttabbed line\n".
" spaced line\n"
expected := "line^M↵\n".
"→tabbed·line↵\n".
"····spaced·line↵\n"
assert := suite.Assert().Equal(input, expected)| @@ -0,0 +1,5 @@ | |||
| --- a.txt | |||
There was a problem hiding this comment.
if input/expected is inside of the test case, this should be not necessary.
server/handler/file_pairs.go
Outdated
|
|
||
| var preprocessors []service.DiffPreprocessorFunc | ||
|
|
||
| if r.URL.Query().Get("showInvisible") != "" { |
There was a problem hiding this comment.
for consistency with front api call, it should be:
if r.URL.Query().Get("showInvisible") == "1" {
| // ReplaceInvisible preprocessor function that replace invisible character with visible onces | ||
| func ReplaceInvisible(content string) string { | ||
| content = strings.Replace(content, " ", "·", -1) | ||
| content = strings.Replace(content, "\t", "→", -1) |
There was a problem hiding this comment.
How about replacing tab with '→ ' to try to keep indentation? I think a single char indentation may make code less readable.
There was a problem hiding this comment.
I'm used to seeing only one → symbol.
If you want more space, we could use ⟶ (⟶ or ⟶).
but whatever you prefer.
There was a problem hiding this comment.
That's odd, I kind of expect any editor to show the symbol and indent. Like VS Code for example:
There was a problem hiding this comment.
I definitely saw only → many times.
carlosms
left a comment
carlosms
left a comment
There was a problem hiding this comment.
Looks good to me, I just left a suggestion for the tab character.
server/handler/file_pairs.go
Outdated
|
|
||
| if r.URL.Query().Get("showInvisible") != "" { | ||
| preprocessors = append(preprocessors, service.DiffReplaceInvisible) | ||
| if r.URL.Query().Get("showInvisible") != "1" { |
There was a problem hiding this comment.
ups! it should be ==
😉
There was a problem hiding this comment.
ahahahaha. 🤦♂️
fixed.
dpordomingo
left a comment
dpordomingo
left a comment
There was a problem hiding this comment.
LGTM
If @carlosms has not an strong opinión about indentation chars, please squash and merge
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
smacker
force-pushed
the
show_spaces_tab_new_lines
branch
from
a278354 to
ceba33c
Compare
5 participants
Fixes: #176