Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Commit

Permalink
fix(set-text-via-diff): base use on option instead of flaky buffer ra…
Browse files Browse the repository at this point in the history
…nge comparison

We were determining whether or not to use setTextViaDiff (more performant) versus the normal
setTextInBufferRange (less performant but allows replacing only a subset of text in a file if
desired) by whether or not the buffer range of the file equaled the buffer range of the text to be
transformed. However, in some cases such as when the editor automatically inserts an ending newline,
these two ranges can become unequal and we revert to using setTextInBufferRange. This is less
performant and not really necessary. The fix is to just pass an option when calling
`executePrettierOnBufferRange` from `formatOnSave`.
  • Loading branch information
robwise committed Dec 31, 2017
1 parent cb966c6 commit 378b6bd
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 21 deletions.
20 changes: 10 additions & 10 deletions dist/executePrettier/executePrettierOnBufferRange.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ var executePrettierOrIntegration = function () {
}();

var executePrettierOnBufferRange = function () {
var _ref2 = (0, _asyncToGenerator3.default)(_regenerator2.default.mark(function _callee2(editor, bufferRange) {
var cursorPositionPriorToFormat, textToTransform, transformed, isTextUnchanged, editorBuffer;
var _ref2 = (0, _asyncToGenerator3.default)(_regenerator2.default.mark(function _callee2(editor, bufferRange, options) {
var cursorPositionPriorToFormat, textToTransform, transformed, isTextUnchanged;
return _regenerator2.default.wrap(function _callee2$(_context2) {
while (1) {
switch (_context2.prev = _context2.next) {
Expand Down Expand Up @@ -132,28 +132,28 @@ var executePrettierOnBufferRange = function () {

case 11:

// we use setTextViaDiff when formatting the entire buffer to improve performance,
// maintain metadata (bookmarks, folds, etc) and eliminate syntax highlight flickering
editorBuffer = editor.getBuffer();

if (editorBuffer.getRange().isEqual(bufferRange)) {
editorBuffer.setTextViaDiff(transformed);
if (options.setTextViaDiff) {
// we use setTextViaDiff when formatting the entire buffer to improve performance,
// maintain metadata (bookmarks, folds, etc) and eliminate syntax highlight flickering
// however, we can't always use it because it replaces all text in the file and sometimes
// we're only editing a sub-selection of the text in a file
editor.getBuffer().setTextViaDiff(transformed);
} else {
editor.setTextInBufferRange(bufferRange, transformed);
}

editor.setCursorScreenPosition(cursorPositionPriorToFormat);
runLinter(editor);

case 15:
case 14:
case 'end':
return _context2.stop();
}
}
}, _callee2, undefined);
}));

return function executePrettierOnBufferRange(_x3, _x4) {
return function executePrettierOnBufferRange(_x3, _x4, _x5) {
return _ref2.apply(this, arguments);
};
}();
Expand Down
2 changes: 1 addition & 1 deletion dist/formatOnSave/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ var _require4 = require('../atomInterface'),
var shouldFormatOnSave = require('./shouldFormatOnSave');

var callAppropriatePrettierExecutor = function callAppropriatePrettierExecutor(editor) {
return isCurrentScopeEmbeddedScope(editor) ? executePrettierOnEmbeddedScripts(editor) : executePrettierOnBufferRange(editor, getBufferRange(editor));
return isCurrentScopeEmbeddedScope(editor) ? executePrettierOnEmbeddedScripts(editor) : executePrettierOnBufferRange(editor, getBufferRange(editor), { setTextViaDiff: true });
};

var formatOnSaveIfAppropriate = _.flow(_.tap(clearLinterErrors), _.cond([[shouldFormatOnSave, callAppropriatePrettierExecutor]]));
Expand Down
17 changes: 11 additions & 6 deletions src/executePrettier/executePrettierOnBufferRange.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ const executePrettierOrIntegration = async (editor: TextEditor, text: string) =>
}
};

const executePrettierOnBufferRange = async (editor: TextEditor, bufferRange: Range) => {
const executePrettierOnBufferRange = async (
editor: TextEditor,
bufferRange: Range,
options?: { setTextViaDiff?: boolean },
) => {
const cursorPositionPriorToFormat = editor.getCursorScreenPosition();
const textToTransform = editor.getTextInBufferRange(bufferRange);
const transformed = await executePrettierOrIntegration(editor, textToTransform);
Expand All @@ -51,11 +55,12 @@ const executePrettierOnBufferRange = async (editor: TextEditor, bufferRange: Ran
return;
}

// we use setTextViaDiff when formatting the entire buffer to improve performance,
// maintain metadata (bookmarks, folds, etc) and eliminate syntax highlight flickering
const editorBuffer = editor.getBuffer();
if (editorBuffer.getRange().isEqual(bufferRange)) {
editorBuffer.setTextViaDiff(transformed);
if (options && options.setTextViaDiff) {
// we use setTextViaDiff when formatting the entire buffer to improve performance,
// maintain metadata (bookmarks, folds, etc) and eliminate syntax highlight flickering
// however, we can't always use it because it replaces all text in the file and sometimes
// we're only editing a sub-selection of the text in a file
editor.getBuffer().setTextViaDiff(transformed);
} else {
editor.setTextInBufferRange(bufferRange, transformed);
}
Expand Down
4 changes: 2 additions & 2 deletions src/executePrettier/executePrettierOnBufferRange.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ it('sets the transformed text in the buffer range', async () => {
expect(editor.setTextInBufferRange).toHaveBeenCalledWith(bufferRangeFixture, 'const foo = 2;');
});

it('sets the transformed text via diff when buffer equals entire range of editor', async () => {
it('sets the transformed text via diff when the option is passed', async () => {
const setTextViaDiffMock = jest.fn();
editor.getBuffer.mockImplementation(() => ({
getRange: () => ({ isEqual: () => true }),
setTextViaDiff: setTextViaDiffMock,
}));

await executePrettierOnBufferRange(editor, bufferRangeFixture);
await executePrettierOnBufferRange(editor, bufferRangeFixture, { setTextViaDiff: true });

expect(prettier.format).toHaveBeenCalledWith('const foo = (2);', { useTabs: false });
expect(setTextViaDiffMock).toHaveBeenCalledWith('const foo = 2;');
Expand Down
2 changes: 1 addition & 1 deletion src/formatOnSave/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const shouldFormatOnSave = require('./shouldFormatOnSave');
const callAppropriatePrettierExecutor = (editor: TextEditor) =>
isCurrentScopeEmbeddedScope(editor)
? executePrettierOnEmbeddedScripts(editor)
: executePrettierOnBufferRange(editor, getBufferRange(editor));
: executePrettierOnBufferRange(editor, getBufferRange(editor), { setTextViaDiff: true });

const formatOnSaveIfAppropriate: TextEditor => void = _.flow(
_.tap(clearLinterErrors),
Expand Down
2 changes: 1 addition & 1 deletion src/formatOnSave/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ it('executes prettier on the buffer range if appropriate and scope is not embedd

formatOnSave(editor);

expect(executePrettierOnBufferRange).toHaveBeenCalledWith(editor, mockRange);
expect(executePrettierOnBufferRange).toHaveBeenCalledWith(editor, mockRange, { setTextViaDiff: true });
});

it('executes prettier on the embedded scripts if appropriate and scope is embedded', () => {
Expand Down

0 comments on commit 378b6bd

Please sign in to comment.