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

Fix MaxNumber incrementFileName bug #50865

Merged
merged 2 commits into from
May 31, 2018
Merged

Fix MaxNumber incrementFileName bug #50865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a33bafc
Fix MaxNumber incrementFileName bug
Krzysztof-Cieslak May 31, 2018
ae97c98
Use `MAX_SAFE_SMALL_INTEGER`
Krzysztof-Cieslak May 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 22 additions & 4 deletions src/vs/workbench/parts/files/electron-browser/fileActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import { RawContextKey, IContextKeyService } from 'vs/platform/contextkey/common
import { Schemas } from 'vs/base/common/network';
import { IDialogService, IConfirmationResult, IConfirmation, getConfirmMessage } from 'vs/platform/dialogs/common/dialogs';
import { INotificationService, Severity } from 'vs/platform/notification/common/notification';
import { Constants } from 'vs/editor/common/core/uint';

export interface IEditableData {
action: IAction;
Expand Down Expand Up @@ -1060,20 +1061,27 @@ function findValidPasteFileTarget(targetFolder: ExplorerItem, fileToPaste: { res

export function incrementFileName(name: string, isFolder: boolean): string {
const separators = '[\\.\\-_]';
const maxNumber = Constants.MAX_SAFE_SMALL_INTEGER;

// file.1.txt=>file.2.txt
let suffixFileRegex = RegExp('(.*' + separators + ')(\\d+)(\\..*)$');
if (!isFolder && name.match(suffixFileRegex)) {
return name.replace(suffixFileRegex, (match, g1?, g2?, g3?) => {
return g1 + strings.pad(parseInt(g2) + 1, g2.length) + g3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a helper method for this?
Since this code seems like it is duplicated 4 times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's bit different for every case since it formats the string in different formats, not sure if there is a sense in trying to extract that to helper function.

let number = parseInt(g2);
return number < maxNumber
? g1 + strings.pad(number + 1, g2.length) + g3
: strings.format('{0}{1}.1{2}', g1, g2, g3);
});
}

// 1.file.txt=>2.file.txt
let prefixFileRegex = RegExp('(\\d+)(' + separators + '.*)(\\..*)$');
if (!isFolder && name.match(prefixFileRegex)) {
return name.replace(prefixFileRegex, (match, g1?, g2?, g3?) => {
return strings.pad(parseInt(g1) + 1, g1.length) + g2 + g3;
let number = parseInt(g1);
return number < maxNumber
? strings.pad(number + 1, g1.length) + g2 + g3
: strings.format('{0}{1}.1{2}', g1, g2, g3);
});
}

Expand All @@ -1085,12 +1093,22 @@ export function incrementFileName(name: string, isFolder: boolean): string {

// folder.1=>folder.2
if (isFolder && name.match(/(\d+)$/)) {
return name.replace(/(\d+)$/, (match: string, ...groups: any[]) => { return strings.pad(parseInt(groups[0]) + 1, groups[0].length); });
return name.replace(/(\d+)$/, (match: string, ...groups: any[]) => {
let number = parseInt(groups[0]);
return number < maxNumber
? strings.pad(number + 1, groups[0].length)
: strings.format('{0}.1', groups[0]);
});
}

// 1.folder=>2.folder
if (isFolder && name.match(/^(\d+)/)) {
return name.replace(/^(\d+)/, (match: string, ...groups: any[]) => { return strings.pad(parseInt(groups[0]) + 1, groups[0].length); });
return name.replace(/^(\d+)(.*)$/, (match: string, ...groups: any[]) => {
let number = parseInt(groups[0]);
return number < maxNumber
? strings.pad(number + 1, groups[0].length) + groups[1]
: strings.format('{0}{1}.1', groups[0], groups[1]);
});
}

// file/folder=>file.1/folder.1
Expand Down
30 changes: 27 additions & 3 deletions src/vs/workbench/parts/files/test/electron-browser/fileActions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,18 @@ suite('Files - Increment file name', () => {
assert.strictEqual(result, 'test_2');
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Great job with tests

test('Increment file name with suffix version, too big number', function () {
const name = 'test.9007199254740992.js';
const result = incrementFileName(name, false);
assert.strictEqual(result, 'test.9007199254740992.1.js');
});

test('Increment folder name with suffix version, too big number', function () {
const name = 'test.9007199254740992';
const result = incrementFileName(name, true);
assert.strictEqual(result, 'test.9007199254740992.1');
});

test('Increment file name with prefix version', function () {
const name = '1.test.js';
const result = incrementFileName(name, false);
Expand All @@ -112,19 +124,31 @@ suite('Files - Increment file name', () => {
assert.strictEqual(result, '2_test.js');
});

test('Increment folder name with suffix version', function () {
test('Increment file name with prefix version, too big number', function () {
const name = '9007199254740992.test.js';
const result = incrementFileName(name, false);
assert.strictEqual(result, '9007199254740992.test.1.js');
});

test('Increment folder name with prefix version', function () {
const name = '1.test';
const result = incrementFileName(name, true);
assert.strictEqual(result, '2.test');
});

test('Increment folder name with suffix version, trailing zeros', function () {
test('Increment folder name with prefix version, too big number', function () {
const name = '9007199254740992.test';
const result = incrementFileName(name, true);
assert.strictEqual(result, '9007199254740992.test.1');
});

test('Increment folder name with prefix version, trailing zeros', function () {
const name = '001.test';
const result = incrementFileName(name, true);
assert.strictEqual(result, '002.test');
});

test('Increment folder name with suffix version with `-` as separator', function () {
test('Increment folder name with prefix version with `-` as separator', function () {
const name = '1-test';
const result = incrementFileName(name, true);
assert.strictEqual(result, '2-test');
Expand Down