ronantakizawa/codereview-bench huggingface dataset:
Repository: atom/atom
File: src/main-process/parse-command-line.js (JavaScript)

Code before:
    pathsToOpen,
    urlsToOpen,
    executedFrom,
    test,
    version,
    pidToKillWhenClosed,
    devMode,
    safeMode,
    newWindow,
    logFile,
    socketPath,
    userDataDir,
    profileStartup,
    timeout,
    clearWindowState,
    addToLastWindow,
    mainProcess,
    benchmark,
    benchmarkTest,
    env: process.env
  }
}

const readProjectSettingsSync = (filepath, executedFrom) => {
  try {
    const readPath = path.isAbsolute(filepath) ? filepath : path.join(executedFrom, filepath)
    const contents = CSON.readFileSync(readPath)
    if (contents.paths && contents.config) {
      return contents
    }
    throw new Error()
  } catch (e) {
    const errorMessage = `Unable to read supplied atomproject file. This file must have a valid array of paths, as well as a valid config object.`
    throw new Error(errorMessage)
  }
}

const relativizeToAtomProject = (curPath, atomProject) => {
  return path.isAbsolute(curPath) ? curPath : path.join(atomProject, curPath)
}

const normalizeDriveLetterName = (filePath) => {
  if (process.platform === 'win32') {
    return filePath.replace(/^([a-z]):/, ([driveLetter]) => driveLetter.toUpperCase() + ':')
  } else {
    return filePath
  }
}


Review comment (bug):
any reason to have this function do the path resolution instead of passing in the path already resolved? it seems like you're doing that work at the callsite anyway.

also, i could be wrong but i think the convention in the codebase is to favor function declaration (i.e. use the "function" keyword) for top-level functions like this, unless it's like a single line implicit return deal.

Code after:
    // explicitly pass it by command line, see http://git.io/YC8_Ew.
    process.env.PATH = args['path-environment']
  }

  resourcePath = normalizeDriveLetterName(resourcePath)
  devResourcePath = normalizeDriveLetterName(devResourcePath)

  return {
    projectSpecification,
    resourcePath,
    devResourcePath,
    pathsToOpen,
    urlsToOpen,
    executedFrom,
    test,
    version,
    pidToKillWhenClosed,
    devMode,
    safeMode,
    newWindow,
    logFile,
    socketPath,
    userDataDir,
    profileStartup,
    timeout,
    clearWindowState,
    addToLastWindow,
    mainProcess,
    benchmark,
    benchmarkTest,
    env: process.env
  }
}

function readProjectSpecificationSync (filepath, executedFrom) {
  let contents
  try {
    contents = CSON.readFileSync(filepath)
  } catch (e) {
    throw new Error('Unable to read supplied project specification file.')
  }

  contents.config = (contents.config == null) ? {} : contents.config
  return contents
}

function normalizeDriveLetterName (filePath) {
  if (process.platform === 'win32') {
    return filePath.replace(/^([a-z]):/, ([driveLetter]) => driveLetter.toUpperCase() + ':')
  } else {
    return filePath
Repository: atom/atom
File: src/main-process/parse-command-line.js (JavaScript)

Code before:
    pidToKillWhenClosed,
    devMode,
    safeMode,
    newWindow,
    logFile,
    socketPath,
    userDataDir,
    profileStartup,
    timeout,
    clearWindowState,
    addToLastWindow,
    mainProcess,
    benchmark,
    benchmarkTest,
    env: process.env
  }
}

const readProjectSettingsSync = (filepath, executedFrom) => {
  try {
    const readPath = path.isAbsolute(filepath) ? filepath : path.join(executedFrom, filepath)
    const contents = CSON.readFileSync(readPath)
    if (contents.paths && contents.config) {
      return contents
    }
    throw new Error()
  } catch (e) {
    const errorMessage = `Unable to read supplied atomproject file. This file must have a valid array of paths, as well as a valid config object.`
    throw new Error(errorMessage)
  }
}

const relativizeToAtomProject = (curPath, atomProject) => {
  return path.isAbsolute(curPath) ? curPath : path.join(atomProject, curPath)
}

const normalizeDriveLetterName = (filePath) => {
  if (process.platform === 'win32') {
    return filePath.replace(/^([a-z]):/, ([driveLetter]) => driveLetter.toUpperCase() + ':')
  } else {
    return filePath
  }
}


Review comment (suggestion):
throwing an empty error and then immediately catching it is kinda weird. you can just move the later throw out of the catch block:

```js
try {
    const contents = CSON.readFileSync(readPath)
    if (contents.paths && contents.config) return contents
} catch (err) {}
throw new Error(...)
```

or maybe even have two separate errors

Code after:
  devResourcePath = normalizeDriveLetterName(devResourcePath)

  return {
    projectSpecification,
    resourcePath,
    devResourcePath,
    pathsToOpen,
    urlsToOpen,
    executedFrom,
    test,
    version,
    pidToKillWhenClosed,
    devMode,
    safeMode,
    newWindow,
    logFile,
    socketPath,
    userDataDir,
    profileStartup,
    timeout,
    clearWindowState,
    addToLastWindow,
    mainProcess,
    benchmark,
    benchmarkTest,
    env: process.env
  }
}

function readProjectSpecificationSync (filepath, executedFrom) {
  let contents
  try {
    contents = CSON.readFileSync(filepath)
  } catch (e) {
    throw new Error('Unable to read supplied project specification file.')
  }

  contents.config = (contents.config == null) ? {} : contents.config
  return contents
}

function normalizeDriveLetterName (filePath) {
  if (process.platform === 'win32') {
    return filePath.replace(/^([a-z]):/, ([driveLetter]) => driveLetter.toUpperCase() + ':')
  } else {
    return filePath
  }
}
Repository: atom/atom
File: src/main-process/parse-command-line.js (JavaScript)

Code before:
    devMode,
    safeMode,
    newWindow,
    logFile,
    socketPath,
    userDataDir,
    profileStartup,
    timeout,
    clearWindowState,
    addToLastWindow,
    mainProcess,
    benchmark,
    benchmarkTest,
    env: process.env
  }
}

function readProjectSpecificationSync (filepath, executedFrom) {
  try {
    const readPath = path.isAbsolute(filepath) ? filepath : path.join(executedFrom, filepath)
    const contents = CSON.readFileSync(readPath)

    if (contents.paths == null) {
      contents.paths = [path.dirname(readPath)]
    }
    if (contents.config) {
      return contents
    }
  } catch (e) {}
  const errorMessage = 'Unable to read supplied project specification file. This file must have a valid array of paths, as well as a valid config object.'
  throw new Error(errorMessage)
}

function normalizeDriveLetterName (filePath) {
  if (process.platform === 'win32') {
    return filePath.replace(/^([a-z]):/, ([driveLetter]) => driveLetter.toUpperCase() + ':')
  } else {
    return filePath
  }
}


Review comment (suggestion):
So, if `contents` does not have a `config` property, we return undefined? Then we are relying on the fact that `Object.assign({}, undefined)` makes an empty object?

Seems like it would be better to either explicitly return an empty object, or throw.

Code after:

  return {
    projectSpecification,
    resourcePath,
    devResourcePath,
    pathsToOpen,
    urlsToOpen,
    executedFrom,
    test,
    version,
    pidToKillWhenClosed,
    devMode,
    safeMode,
    newWindow,
    logFile,
    socketPath,
    userDataDir,
    profileStartup,
    timeout,
    clearWindowState,
    addToLastWindow,
    mainProcess,
    benchmark,
    benchmarkTest,
    env: process.env
  }
}

function readProjectSpecificationSync (filepath, executedFrom) {
  let contents
  try {
    contents = CSON.readFileSync(filepath)
  } catch (e) {
    throw new Error('Unable to read supplied project specification file.')
  }

  contents.config = (contents.config == null) ? {} : contents.config
  return contents
}

function normalizeDriveLetterName (filePath) {
  if (process.platform === 'win32') {
    return filePath.replace(/^([a-z]):/, ([driveLetter]) => driveLetter.toUpperCase() + ':')
  } else {
    return filePath
  }
}
Repository: atom/atom
File: src/atom-environment.js (JavaScript)

Code before:

    ipcRenderer.send('window-command', 'window:locations-opened')
  }

  resolveProxy (url) {
    return new Promise((resolve, reject) => {
      const requestId = this.nextProxyRequestId++
      const disposable = this.applicationDelegate.onDidResolveProxy((id, proxy) => {
        if (id === requestId) {
          disposable.dispose()
          resolve(proxy)
        }
      })

      return this.applicationDelegate.resolveProxy(requestId, url)
    })
  }
}

AtomEnvironment.version = 1
AtomEnvironment.prototype.saveStateDebounceInterval = 1000
module.exports = AtomEnvironment

/* eslint-disable */

// Preserve this deprecation until 2.0. Sorry. Should have removed Q sooner.
Promise.prototype.done = function (callback) {
  deprecate('Atom now uses ES6 Promises instead of Q. Call promise.then instead of promise.done')
  return this.then(callback)
}

/* eslint-enable */


Review comment (suggestion):
Not a big deal, but you could probably just include this case into the `else` by trimming the `, ` off of the end of the description string when `length === 2` before adding the "and" part, but tomato tomahto :)

Code after:
        for (let folder of foldersToAddToProject) {
          this.project.addPath(folder)
        }
      }
    }

    if (!restoredState) {
      const fileOpenPromises = []
      for (const {pathToOpen, initialLine, initialColumn} of fileLocationsToOpen) {
        fileOpenPromises.push(this.workspace && this.workspace.open(pathToOpen, {initialLine, initialColumn}))
      }
      await Promise.all(fileOpenPromises)
    }

    if (missingFolders.length > 0) {
      let message = 'Unable to open project folder'
      if (missingFolders.length > 1) {
        message += 's'
      }

      let description = 'The '
      if (missingFolders.length === 1) {
        description += 'directory `'
        description += missingFolders[0].pathToOpen
        description += '` does not exist.'
      } else if (missingFolders.length === 2) {
        description += `directories \`${missingFolders[0].pathToOpen}\` `
        description += `and \`${missingFolders[1].pathToOpen}\` do not exist.`
      } else {
        description += 'directories '
        description += (missingFolders
          .slice(0, -1)
          .map(location => location.pathToOpen)
          .map(pathToOpen => '`' + pathToOpen + '`, ')
          .join(''))
        description += 'and `' + missingFolders[missingFolders.length - 1].pathToOpen + '` do not exist.'
      }

      this.notifications.addWarning(message, {description})
    }

    ipcRenderer.send('window-command', 'window:locations-opened')
  }

  resolveProxy (url) {
    return new Promise((resolve, reject) => {
      const requestId = this.nextProxyRequestId++
      const disposable = this.applicationDelegate.onDidResolveProxy((id, proxy) => {
        if (id === requestId) {
          disposable.dispose()
          resolve(proxy)
Repository: atom/atom
File: src/tree-sitter-language-mode.js (JavaScript)

Code before:
            grammar,
            this.depth + 1,
            injectionPoint.includeChildren
          );
          marker.parentLanguageLayer = this;
        }

        markersToUpdate.set(
          marker,
          new NodeRangeSet(
            nodeRangeSet,
            injectionNodes,
            injectionPoint.newlinesBetween,
            injectionPoint.includeChildren
          )
        );
      }
    }

    for (const marker of existingInjectionMarkers) {
      if (!markersToUpdate.has(marker)) {
        marker.languageLayer.destroy();
        this.languageMode.emitRangeUpdate(marker.getRange());
        marker.destroy();
      }
    }

    if (markersToUpdate.size > 0) {
      const promises = [];
      for (const [marker, nodeRangeSet] of markersToUpdate) {
        promises.push(marker.languageLayer.update(nodeRangeSet));
      }
      return Promise.all(promises);
    }
  }

  _treeEditForBufferChange(start, oldEnd, newEnd, oldText, newText) {
    const startIndex = this.languageMode.buffer.characterIndexForPosition(
      start
    );
    return {
      startIndex,
      oldEndIndex: startIndex + oldText.length,
      newEndIndex: startIndex + newText.length,
      startPosition: start,
      oldEndPosition: oldEnd,
      newEndPosition: newEnd
    };
  }
}


Review comment (suggestion):
Was this line removal intentional? I am not super familiar with this code but just double-checking in case it was removed accidentally, as it seems orthogonal to the other changes in this pull request.

Code after:
            this.depth + 1
          );
          marker.parentLanguageLayer = this;
        }

        markersToUpdate.set(
          marker,
          new NodeRangeSet(
            nodeRangeSet,
            injectionNodes,
            injectionPoint.newlinesBetween,
            injectionPoint.includeChildren
          )
        );
      }
    }

    for (const marker of existingInjectionMarkers) {
      if (!markersToUpdate.has(marker)) {
        marker.languageLayer.destroy();
        this.languageMode.emitRangeUpdate(marker.getRange());
        marker.destroy();
      }
    }

    if (markersToUpdate.size > 0) {
      const promises = [];
      for (const [marker, nodeRangeSet] of markersToUpdate) {
        promises.push(marker.languageLayer.update(nodeRangeSet));
      }
      return Promise.all(promises);
    }
  }

  _treeEditForBufferChange(start, oldEnd, newEnd, oldText, newText) {
    const startIndex = this.languageMode.buffer.characterIndexForPosition(
      start
    );
    return {
      startIndex,
      oldEndIndex: startIndex + oldText.length,
      newEndIndex: startIndex + newText.length,
      startPosition: start,
      oldEndPosition: oldEnd,
      newEndPosition: newEnd
    };
  }
}

class HighlightIterator {
  constructor(languageMode) {
Repository: atom/atom
File: src/tree-sitter-language-mode.js (JavaScript)

Code before:
      while (i > 0 && this.iterators[i - 1].compare(leader) < 0) i--;
      if (i < leaderIndex) {
        this.iterators.splice(i, 0, this.iterators.pop());
      }
    } else {
      // If the layer iterator was at the end of its syntax tree, then remove
      // it from the array.
      this.iterators.pop();
    }

    this.detectCoveredScope();
  }

  // Detect whether or not another more deeply-nested language layer has a
  // scope boundary at this same position. If so, the current language layer's
  // scope boundary should not be reported.
  detectCoveredScope() {
    const layerCount = this.iterators.length;
    if (layerCount > 1) {
      const first = this.iterators[layerCount - 1];
      const next = this.iterators[layerCount - 2];
      if (
        next.offset === first.offset &&
        next.atEnd === first.atEnd &&
        next.depth > first.depth
      ) {
        this.currentScopeIsCovered = true;
        return;
      }
    }
    this.currentScopeIsCovered = false;
  }

  getPosition() {
    const iterator = last(this.iterators);
    if (iterator) {
      return iterator.getPosition();
    } else {
      return Point.INFINITY;
    }
  }

  getCloseScopeIds() {
    const iterator = last(this.iterators);
    if (iterator && !this.currentScopeIsCovered) {
      return iterator.getCloseScopeIds();
    }
    return [];
  }

  getOpenScopeIds() {

Review comment (suggestion):
Oh wow you ended up with a really tiny diff! I think this fix makes sense.

Down the road, if this type of problem occurs in other cases, we'll probably just have to make this conditional even more specific.

Code after:
      while (i > 0 && this.iterators[i - 1].compare(leader) < 0) i--;
      if (i < leaderIndex) {
        this.iterators.splice(i, 0, this.iterators.pop());
      }
    } else {
      // If the layer iterator was at the end of its syntax tree, then remove
      // it from the array.
      this.iterators.pop();
    }

    this.detectCoveredScope();
  }

  // Detect whether or not another more deeply-nested language layer has a
  // scope boundary at this same position. If so, the current language layer's
  // scope boundary should not be reported.
  detectCoveredScope() {
    const layerCount = this.iterators.length;
    if (layerCount > 1) {
      const first = this.iterators[layerCount - 1];
      const next = this.iterators[layerCount - 2];
      if (
        next.offset === first.offset &&
        next.atEnd === first.atEnd &&
        next.depth > first.depth &&
        next.openTags.length + next.closeTags.length > 0
      ) {
        this.currentScopeIsCovered = true;
        return;
      }
    }
    this.currentScopeIsCovered = false;
  }

  getPosition() {
    const iterator = last(this.iterators);
    if (iterator) {
      return iterator.getPosition();
    } else {
      return Point.INFINITY;
    }
  }

  getCloseScopeIds() {
    const iterator = last(this.iterators);
    if (iterator && !this.currentScopeIsCovered) {
      return iterator.getCloseScopeIds();
    }
    return [];
  }
Repository: atom/atom
File: src/module-utils.js (JavaScript)

Code before:
// a require function with both ES5 and ES6 default export support
function requireX(path) {
  const modul = require(path);
  if (modul === null || modul === undefined) {
    // if null do not bother
    return modul;
  } else {
    if (
      modul.__esModule === true &&
      (modul.default !== undefined || modul.default !== null)
    ) {
      // __esModule flag is true and default is exported, which means that
      // an object containing the main functions (e.g. activate, etc) is default exported
      return modul.default;
    } else {
      return modul;
    }
  }
}

exports.requireX = requireX;


Review comment (suggestion):
This will always be true since `modul.default` can not be `undefined` and `null`.

Also this check should not be necessary. if a module wants to export `null` or `undefined` than I would rather the return be `undefined` or `null` than `{default: undefined}` or `{default: null}`.

If a module exports `__esModule = true` but doesn't provide a `default` export than this should return `undefined`. 

Code after:
// a require function with both ES5 and ES6 default export support
function requireModule(path) {
  const modul = require(path);
  if (modul === null || modul === undefined) {
    // if null do not bother
    return modul;
  } else {
    if (
      modul.__esModule === true &&
      (modul.default !== undefined && modul.default !== null)
    ) {
      // __esModule flag is true and default is exported, which means that
      // an object containing the main functions (e.g. activate, etc) is default exported
      return modul.default;
    } else {
      return modul;
    }
  }
}

exports.requireModule = requireModule;
Repository: atom/atom
File: src/module-utils.js (JavaScript)

Code before:
// a require function with both ES5 and ES6 default export support
function requireX(path) {
  const modul = require(path);
  if (modul === null || modul === undefined) {
    // if null do not bother
    return modul;
  } else {
    if (
      modul.__esModule === true &&
      (modul.default !== undefined || modul.default !== null)
    ) {
      // __esModule flag is true and default is exported, which means that
      // an object containing the main functions (e.g. activate, etc) is default exported
      return modul.default;
    } else {
      return modul;
    }
  }
}

exports.requireX = requireX;


Review comment (suggestion):
I think a more descriptive name should be used. When someone sees this function called in the code they should have some idea of what it does and why it is used.

Something like: `requirePossibleEsModule` or `requireDefault`

Code after:
// a require function with both ES5 and ES6 default export support
function requireModule(path) {
  const modul = require(path);
  if (modul === null || modul === undefined) {
    // if null do not bother
    return modul;
  } else {
    if (
      modul.__esModule === true &&
      (modul.default !== undefined && modul.default !== null)
    ) {
      // __esModule flag is true and default is exported, which means that
      // an object containing the main functions (e.g. activate, etc) is default exported
      return modul.default;
    } else {
      return modul;
    }
  }
}

exports.requireModule = requireModule;
Repository: atom/atom
File: src/main-process/atom-window.js (JavaScript)

Code before:
  }

  isSpecWindow() {
    return this.isSpec;
  }

  reload() {
    this.loadedPromise = new Promise(resolve => {
      this.resolveLoadedPromise = resolve;
    });
    this.prepareToUnload().then(canUnload => {
      if (canUnload) this.browserWindow.reload();
    });
    return this.loadedPromise;
  }

  showSaveDialog(options, callback) {
    options = Object.assign(
      {
        title: 'Save File',
        defaultPath: this.projectRoots[0]
      },
      options
    );

    if (typeof callback === 'function') {
      // Async
      dialog.showSaveDialog(this.browserWindow, options, callback);
    } else {
      // Sync
      return dialog.showSaveDialog(this.browserWindow, options);
    }
  }

  toggleDevTools() {
    return this.browserWindow.toggleDevTools();
  }

  openDevTools() {
    return this.browserWindow.openDevTools();
  }

  closeDevTools() {
    return this.browserWindow.closeDevTools();
  }

  setDocumentEdited(documentEdited) {
    return this.browserWindow.setDocumentEdited(documentEdited);
  }

  setRepresentedFilename(representedFilename) {

Review comment (bug):
This line will probably fail linting. (It has an extra space ` `.) It needs this fix: https://github.com/atom/atom/pull/21935/commits/2f0711425d321955aa17c11a665e3089bd1e8b42

(@asturur did that for the other PR where the tests were running. But it was less obvious here.)

Code after:
  }

  isSpecWindow() {
    return this.isSpec;
  }

  reload() {
    this.loadedPromise = new Promise(resolve => {
      this.resolveLoadedPromise = resolve;
    });
    this.prepareToUnload().then(canUnload => {
      if (canUnload) this.browserWindow.reload();
    });
    return this.loadedPromise;
  }

  showSaveDialog(options, callback) {
    options = Object.assign(
      {
        title: 'Save File',
        defaultPath: this.projectRoots[0]
      },
      options
    );

    let promise =  dialog.showSaveDialog(this.browserWindow, options);
    if (typeof callback === 'function') {
      promise = promise.then(({ filePath, bookmark }) => {
        callback(filePath, bookmark);
      });
    }
    return promise;
  }

  toggleDevTools() {
    return this.browserWindow.toggleDevTools();
  }

  openDevTools() {
    return this.browserWindow.openDevTools();
  }

  closeDevTools() {
    return this.browserWindow.closeDevTools();
  }

  setDocumentEdited(documentEdited) {
    return this.browserWindow.setDocumentEdited(documentEdited);
  }

  setRepresentedFilename(representedFilename) {