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

Escape path for the file list #22741

Merged
merged 5 commits into from
Feb 4, 2023
Merged

Escape path for the file list #22741

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
56d5479
escape path
wxiaoguang Feb 3, 2023
281b0c3
Merge branch 'main' into js-escape-path
wxiaoguang Feb 4, 2023
947f01b
Merge branch 'main' into js-escape-path
wxiaoguang Feb 4, 2023
1b4551e
Merge branch 'main' into js-escape-path
lunny Feb 4, 2023
794de46
Merge branch 'main' into js-escape-path
lunny Feb 4, 2023
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
6 changes: 5 additions & 1 deletion web_src/js/features/repo-findfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ export function filterRepoFilesWeighted(files, filter) {
return filterResult;
}

export function escapePath(s) {
return s.split('/').map(encodeURIComponent).join('/');
}
Copy link
Member

@silverwind silverwind Feb 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Move function to utils.js? I think such simple pure functions belong there so they can be imported everywere easily without circular dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have thought about it, but I didn't do it for 2 reasons:

  1. Now the escapePath is only used by repo-findfile.js, not sure whether it is general enough for frontend.
  2. The utils.js have already contained many different functions, maybe in the future it should be refactored to utils/color.js, utils/string.js, utils/html.js, etc. I do not feel it's good to add too many functions to it now.

So, I prefer to keep the escapePath here. If it's really needed to be used somewhere else, let's do refactor.


function filterRepoFiles(filter) {
const treeLink = $repoFindFileInput.attr('data-url-tree-link');
$repoFindFileTableBody.empty();
Expand All @@ -83,7 +87,7 @@ function filterRepoFiles(filter) {
for (const r of filterResult) {
const $row = $(tmplRow);
const $a = $row.find('a');
$a.attr('href', `${treeLink}/${r.matchResult.join('')}`);
$a.attr('href', `${treeLink}/${escapePath(r.matchResult.join(''))}`);
const $octiconFile = $(svg('octicon-file')).addClass('mr-3');
$a.append($octiconFile);
// if the target file path is "abc/xyz", to search "bx", then the matchResult is ['a', 'b', 'c/', 'x', 'yz']
Expand Down
7 changes: 6 additions & 1 deletion web_src/js/features/repo-findfile.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {describe, expect, test} from 'vitest';
import {strSubMatch, calcMatchedWeight, filterRepoFilesWeighted} from './repo-findfile.js';
import {strSubMatch, calcMatchedWeight, filterRepoFilesWeighted, escapePath} from './repo-findfile.js';

describe('Repo Find Files', () => {
test('strSubMatch', () => {
Expand Down Expand Up @@ -32,4 +32,9 @@ describe('Repo Find Files', () => {
expect(res).toHaveLength(2);
expect(res[0].matchResult).toEqual(['', 'word', '.txt']);
});

test('escapePath', () => {
expect(escapePath('a/b/c')).toEqual('a/b/c');
expect(escapePath('a/b/ c')).toEqual('a/b/%20c');
Copy link
Member

Choose a reason for hiding this comment

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

How about add a test for #?

Copy link
Contributor Author

@wxiaoguang wxiaoguang Feb 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's not necessary .... encodeURIComponent already does the right thing. Otherwise there will be a lot like =, +, @, : ... etc .....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From another perspective, if a users reports about + in file name doesn't work, do you want to test the +? And if another reports % doesn't work, do you want to test the %?

If you want to test encodeURIComponent, then every char execpt -_~. should be tested, I can not see the necessary.

Test cases are designed for what it should be , but not what users said what happened.

I can see many test cases in old code are not well designed, sometimes some developers just copy complex contents as test case data, which make the cases difficult to be maintained, and make future developers can not understand what the cases are for.

In a word: keep things simple and focused.

});
});