ronantakizawa/codereview-bench huggingface dataset:
Repository: angular/angular.js
File: scripts/travis/build.sh (Shell)

Code before:
    grunt test:travis-protractor --specs="$TARGET_SPECS"
    ;;
  "deploy")
    export DEPLOY_DOCS
    export DEPLOY_CODE

    DIST_TAG=$( jq ".distTag" "package.json" | tr -d "\"[:space:]" )

    # upload docs if the branch distTag from package.json is "latest" (i.e. stable branch)
    if [[ "$DIST_TAG" == latest ]]; then
      DEPLOY_DOCS=true
    else
      DEPLOY_DOCS=false
    fi

    # upload the build (code + docs) if ...
    #   the commit is tagged
    #   - or the branch is "master"
    #   - or the branch distTag from package.json is "latest" (i.e. stable branch)
    if [[ "$TRAVIS_TAG" != '' || "$TRAVIS_BRANCH" == master || "$DIST_TAG" == latest ]]; then
      DEPLOY_CODE=true
    else
      DEPLOY_CODE=false
    fi

    if [[ "$DEPLOY_DOCS" || "$DEPLOY_CODE" ]]; then
      grunt prepareFirebaseDeploy
    else
      echo "Skipping deployment build because conditions have not been met."
    fi
    ;;
  *)
    echo "Unknown job type. Please set JOB=ci-checks, JOB=unit, JOB=deploy or JOB=e2e-*."
    ;;
esac

Review comment (suggestion):
I think this will always evaluate to true, even if both are `false` (because `false` is truthy :smiley:)

Code after:
    grunt test:travis-protractor --specs="$TARGET_SPECS"
    ;;
  "deploy")
    export DEPLOY_DOCS
    export DEPLOY_CODE

    DIST_TAG=$( jq ".distTag" "package.json" | tr -d "\"[:space:]" )

    # upload docs if the branch distTag from package.json is "latest" (i.e. stable branch)
    if [[ "$DIST_TAG" == latest ]]; then
      DEPLOY_DOCS=true
    else
      DEPLOY_DOCS=false
    fi

    # upload the build (code + docs) if ...
    #   the commit is tagged
    #   - or the branch is "master"
    #   - or the branch distTag from package.json is "latest" (i.e. stable branch)
    if [[ "$TRAVIS_TAG" != '' || "$TRAVIS_BRANCH" == master || "$DIST_TAG" == latest ]]; then
      DEPLOY_CODE=true
    else
      DEPLOY_CODE=false
    fi

    if [[ "$DEPLOY_DOCS" == true || "$DEPLOY_CODE" == true ]]; then
      grunt prepareFirebaseDeploy
    else
      echo "Skipping deployment build because conditions have not been met."
    fi
    ;;
  *)
    echo "Unknown job type. Please set JOB=ci-checks, JOB=unit, JOB=deploy or JOB=e2e-*."
    ;;
esac
Repository: angular/angular.js
File: docs/config/processors/sitemap.js (JavaScript)

Code before:
'use strict';

module.exports = function createSitemap() {
  return {
    $runAfter: ['paths-computed'],
    $runBefore: ['rendering-docs'],
    $process: function(docs) {
      docs.push({
        id: 'sitemap.xml',
        path: 'sitemap.xml',
        outputPath: '../sitemap.xml',
        template: 'sitemap.template.xml',
        urls: docs.filter(function(doc) {
          return doc.path &&
            doc.outputPath &&
            doc.outputPath.indexOf('index') !== 0 &&
            doc.outputPath.indexOf('examples/') !== 0 &&
            doc.outputPath.indexOf('ptore2e/') !== 0;
        }).map(function(doc) {
          return doc.path;
        })
      });
    }
  };
};


Review comment (suggestion):
You could do this with a regex:

```
!/^index|examples\/|ptore2e\//.test(doc.outputPath)
```

(@gkalpak would be proud of me :-D )

Code after:
'use strict';

var exclusionRegex = /^index|examples\/|ptore2e\//;

module.exports = function createSitemap() {
  return {
    $runAfter: ['paths-computed'],
    $runBefore: ['rendering-docs'],
    $process: function(docs) {
      docs.push({
        id: 'sitemap.xml',
        path: 'sitemap.xml',
        outputPath: '../sitemap.xml',
        template: 'sitemap.template.xml',
        urls: docs.filter(function(doc) {
          return doc.path &&
            doc.outputPath &&
            !exclusionRegex.test(doc.outputPath);
        }).map(function(doc) {
          return doc.path;
        })
      });
    }
  };
};
Repository: angular/angular.js
File: src/ng/directive/form.js (JavaScript)

Code before:
'use strict';

/* global -nullFormCtrl, -PENDING_CLASS, -SUBMITTED_CLASS
 */
var nullFormCtrl = {
  $addControl: noop,
  $getControls: noop,
  $$renameControl: nullFormRenameControl,
  $removeControl: noop,
  $setValidity: noop,
  $setDirty: noop,
  $setPristine: noop,
  $setSubmitted: noop,
  $$setSubmitted: noop
},
PENDING_CLASS = 'ng-pending',
SUBMITTED_CLASS = 'ng-submitted';

function nullFormRenameControl(control, name) {
  control.$name = name;
}

/**
 * @ngdoc type
 * @name form.FormController
 *
 * @property {boolean} $pristine True if user has not interacted with the form yet.
 * @property {boolean} $dirty True if user has already interacted with the form.
 * @property {boolean} $valid True if all of the containing forms and controls are valid.
 * @property {boolean} $invalid True if at least one containing control or form is invalid.
 * @property {boolean} $submitted True if user has submitted the form even if its invalid.
 *

Review comment (suggestion):
I think this should return an empty array (to respect the "contract" that `$getControls()` returns an array).
Maybe change it to `valueFn([])`.

Code after:
'use strict';

/* global -nullFormCtrl, -PENDING_CLASS, -SUBMITTED_CLASS
 */
var nullFormCtrl = {
  $addControl: noop,
  $getControls: valueFn([]),
  $$renameControl: nullFormRenameControl,
  $removeControl: noop,
  $setValidity: noop,
  $setDirty: noop,
  $setPristine: noop,
  $setSubmitted: noop,
  $$setSubmitted: noop
},
PENDING_CLASS = 'ng-pending',
SUBMITTED_CLASS = 'ng-submitted';

function nullFormRenameControl(control, name) {
  control.$name = name;
}

/**
 * @ngdoc type
 * @name form.FormController
 *
 * @property {boolean} $pristine True if user has not interacted with the form yet.
 * @property {boolean} $dirty True if user has already interacted with the form.
 * @property {boolean} $valid True if all of the containing forms and controls are valid.
 * @property {boolean} $invalid True if at least one containing control or form is invalid.
 * @property {boolean} $submitted True if user has submitted the form even if its invalid.
 *
Repository: angular/angular.js
File: test/e2e/fixtures/directive-require-html/script.js (JavaScript)

Code before:
'use strict';

angular.
  module('test', []).
  directive('requireDirective', function() {
    return {
      require: '^^requireTargetDirective',
      link: function(scope, element, attrs, ctrl) {
        window.document.querySelector('#container').append(window.document.createTextNode(ctrl.content));
      }
    };
  }).
  directive('requireTargetDirective', function() {
    return {
      controller: function() {
        this.content = 'requiredContent';
      }
    };
  });



Review comment (suggestion):
I think it would be better if you overwrote `$exceptionHandler` to put the error message into a DOM element.
Then you could easily read the error in the test (and verify that it is what you expect).

Code after:
'use strict';

angular.
  module('test', []).
    provider('$exceptionHandler', /** @this */ function() {
      this.$get = [function() {
        return function(error) {
          window.document.querySelector('#container').textContent = error && error.message;
        };
      }];
  }).

  directive('requireDirective', function() {
    return {
      require: '^^requireTargetDirective',
      link: function(scope, element, attrs, ctrl) {
        window.document.querySelector('#container').textContent = ctrl.content;
      }
    };
  }).
  directive('requireTargetDirective', function() {
    return {
      controller: function() {
        this.content = 'requiredContent';
      }
    };
  });
Repository: angular/angular.js
File: src/ng/sanitizeUri.js (JavaScript)

Code before:
   * If the URL matches the `aImgSanitizationWhitelist` regular expression, it is returned unchanged.
   *
   * If there is no match the URL is returned prefixed with `'unsafe:'` to ensure that when it is written
   * to the DOM it is inactive and potentially malicious code will not be executed.
   *
   * @param {RegExp=} regexp New regexp to whitelist urls with.
   * @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
   *    chaining otherwise.
   */
  this.imgSrcSanitizationWhitelist = function(regexp) {
    if (isDefined(regexp)) {
      imgSrcSanitizationWhitelist = regexp;
      return this;
    }
    return imgSrcSanitizationWhitelist;
  };

  this.$get = function() {
    return function sanitizeUri(uri, isMediaUrl) {
      // if (!uri) return uri;
      var regex = isMediaUrl ? imgSrcSanitizationWhitelist : aHrefSanitizationWhitelist;
      var normalizedVal = urlResolve(uri && uri.toString().trim()).href;
      if (normalizedVal !== '' && !normalizedVal.match(regex)) {
        return 'unsafe:' + normalizedVal;
      }
      return uri.toString();
    };
  };
}


Review comment (bug):
For the record, this will allow more types than just `number`. (This might be a good thing, but it might also lead to unexpected results, instead of a fastfailure.)

If you want to explicitly only allow numbers and strings, you can have an explicit check (as we do in [$anchorScroll][1] for example).

[1]: https://github.com/angular/angular.js/blob/ebeb1c94917eaf53358ae0892232e6a7bb987ce2/src/ng/anchorScroll.js#L242

Code after:
   * If the URL matches the `aImgSanitizationWhitelist` regular expression, it is returned unchanged.
   *
   * If there is no match the URL is returned prefixed with `'unsafe:'` to ensure that when it is written
   * to the DOM it is inactive and potentially malicious code will not be executed.
   *
   * @param {RegExp=} regexp New regexp to whitelist urls with.
   * @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
   *    chaining otherwise.
   */
  this.imgSrcSanitizationWhitelist = function(regexp) {
    if (isDefined(regexp)) {
      imgSrcSanitizationWhitelist = regexp;
      return this;
    }
    return imgSrcSanitizationWhitelist;
  };

  this.$get = function() {
    return function sanitizeUri(uri, isMediaUrl) {
      // if (!uri) return uri;
      var regex = isMediaUrl ? imgSrcSanitizationWhitelist : aHrefSanitizationWhitelist;
      var normalizedVal = urlResolve(uri && uri.trim()).href;
      if (normalizedVal !== '' && !normalizedVal.match(regex)) {
        return 'unsafe:' + normalizedVal;
      }
      return uri;
    };
  };
}
Repository: angular/angular.js
File: src/ng/directive/ngRepeat.js (JavaScript)

Code before:
              previousNode = getBlockEnd(block);
              updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength);
            } else {
              // new item which we don't know about
              $transclude(function ngRepeatTransclude(clone, scope) {
                block.scope = scope;
                // http://jsperf.com/clone-vs-createcomment
                var endNode = ngRepeatEndComment.cloneNode(false);
                clone[clone.length++] = endNode;

                $animate.enter(clone, null, previousNode);
                previousNode = endNode;
                // Note: We only need the first/last node of the cloned nodes.
                // However, we need to keep the reference to the jqlite wrapper as it might be changed later
                // by a directive with templateUrl when its template arrives.
                block.clone = clone;
                nextBlockMap[block.id] = block;
                updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength);
              });
            }
          }
          lastBlockMap = nextBlockMap;

          // Clear the value property from the hashFnLocals object to prevent a reference to the last value
          // being leaked into this ngRepeatCompile function scope
          hashFnLocals[valueIdentifier] = undefined;
        });
      };
    }
  };
}];


Review comment (suggestion):
WDYT about clearing the value here? This way we don't have to touch the `trackByIdExpFn` method that gets invoked on each element and just add this one assignment at the end.

Code after:
              } while (nextNode && nextNode[NG_REMOVED]);

              if (getBlockStart(block) !== nextNode) {
                // existing item which got moved
                $animate.move(getBlockNodes(block.clone), null, previousNode);
              }
              previousNode = getBlockEnd(block);
              updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength);
            } else {
              // new item which we don't know about
              $transclude(function ngRepeatTransclude(clone, scope) {
                block.scope = scope;
                // http://jsperf.com/clone-vs-createcomment
                var endNode = ngRepeatEndComment.cloneNode(false);
                clone[clone.length++] = endNode;

                $animate.enter(clone, null, previousNode);
                previousNode = endNode;
                // Note: We only need the first/last node of the cloned nodes.
                // However, we need to keep the reference to the jqlite wrapper as it might be changed later
                // by a directive with templateUrl when its template arrives.
                block.clone = clone;
                nextBlockMap[block.id] = block;
                updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength);
              });
            }
          }
          lastBlockMap = nextBlockMap;
        });
      };
    }
  };
}];
Repository: angular/angular.js
File: src/ng/directive/ngRepeat.js (JavaScript)

Code before:
            } else {
              // new item which we don't know about
              $transclude(function ngRepeatTransclude(clone, scope) {
                block.scope = scope;
                // http://jsperf.com/clone-vs-createcomment
                var endNode = ngRepeatEndComment.cloneNode(false);
                clone[clone.length++] = endNode;

                $animate.enter(clone, null, previousNode);
                previousNode = endNode;
                // Note: We only need the first/last node of the cloned nodes.
                // However, we need to keep the reference to the jqlite wrapper as it might be changed later
                // by a directive with templateUrl when its template arrives.
                block.clone = clone;
                nextBlockMap[block.id] = block;
                updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength);
              });
            }
          }
          lastBlockMap = nextBlockMap;

          // Clear the value property from the hashFnLocals object to prevent a reference to the last value
          // being leaked into this ngRepeatCompile function scope
          if (hashFnLocals) {
            hashFnLocals[valueIdentifier] = undefined;
          }
        });
      };
    }
  };
}];


Review comment (suggestion):
I would consider moving this right after the `locate existing items` `for` block (since this is whare `hasFnLocals` is used).

Code after:
              if (getBlockStart(block) !== nextNode) {
                // existing item which got moved
                $animate.move(getBlockNodes(block.clone), null, previousNode);
              }
              previousNode = getBlockEnd(block);
              updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength);
            } else {
              // new item which we don't know about
              $transclude(function ngRepeatTransclude(clone, scope) {
                block.scope = scope;
                // http://jsperf.com/clone-vs-createcomment
                var endNode = ngRepeatEndComment.cloneNode(false);
                clone[clone.length++] = endNode;

                $animate.enter(clone, null, previousNode);
                previousNode = endNode;
                // Note: We only need the first/last node of the cloned nodes.
                // However, we need to keep the reference to the jqlite wrapper as it might be changed later
                // by a directive with templateUrl when its template arrives.
                block.clone = clone;
                nextBlockMap[block.id] = block;
                updateScope(block.scope, index, valueIdentifier, value, keyIdentifier, key, collectionLength);
              });
            }
          }
          lastBlockMap = nextBlockMap;
        });
      };
    }
  };
}];
Repository: angular/angular.js
File: src/ngParseExt/module.js (JavaScript)

Code before:
'use strict';

/* eslint-disable new-cap */

/**
 * @ngdoc module
 * @name ngParseExt
 * @packageName angular-parse-ext
 *
 * @knownIssue
 * You cannot use Unicode characters for variable names in the {@link ngRepeat} or {@link ngOptions}
 * expressions (e.g. `ng-repeat="f in поля"`), because they don't use the {@link `$parse`} service
 * to parse the expression.
 *
 * @description
 *
 * The `ngParseExt` module provides functionality to allow Unicode characters in
 * identifiers inside AngularJS expressions.
 *
 * This module allows the usage of any identifier that follows ES6 identifier naming convention
 * to be used as an identifier in an AngularJS expression. ES6 delegates some of the identifier
 * rules definition to Unicode, this module uses ES6 and Unicode 8.0 identifiers convention.
 *
 */

/* global angularParseExtModule: true,
  IDS_Y,
  IDC_Y
*/

function isValidIdentifierStart(ch, cp) {
  return ch === '$' ||
         ch === '_' ||
         IDS_Y(cp);
}

function isValidIdentifierContinue(ch, cp) {
  return ch === '$' ||

Review comment (suggestion):
I think this needs a bit of rewriting to be clear.

> You cannot use Unicode characters for variable names in the {@link ngRepeat} or {@link ngOptions} expressions (e.g. `ng-repeat="f in поля"`). This can be surprising because the {@link `$parse`} service can recognize these characters but these expressions are not parsed by `$parse`.

Code after:
'use strict';

/* eslint-disable new-cap */

/**
 * @ngdoc module
 * @name ngParseExt
 * @packageName angular-parse-ext
 *
 * @description
 *
 * The `ngParseExt` module provides functionality to allow Unicode characters in
 * identifiers inside AngularJS expressions.
 *
 * This module allows the usage of any identifier that follows ES6 identifier naming convention
 * to be used as an identifier in an AngularJS expression. ES6 delegates some of the identifier
 * rules definition to Unicode, this module uses ES6 and Unicode 8.0 identifiers convention.
 *
 * <div class="alert alert-warning">
 * You cannot use Unicode characters for variable names in the {@link ngRepeat} or {@link ngOptions}
 * expressions (e.g. `ng-repeat="f in поля"`), because even with `ngParseExt` included, these
 * special expressions are not parsed by the {@link $parse} service.
 * </div>
 */

/* global angularParseExtModule: true,
  IDS_Y,
  IDC_Y
*/

function isValidIdentifierStart(ch, cp) {
  return ch === '$' ||
         ch === '_' ||
         IDS_Y(cp);
}

function isValidIdentifierContinue(ch, cp) {
  return ch === '$' ||
Repository: ansible/awx
File: awx/sso/tests/unit/test_fields.py (Python)

Code before:
         ['Invalid key(s): "bob", "scooter".']),
        ('MemberDNGroupType', {'name_attr': 'user', 'member_attr': 'west', 'bob': ['a', 'b'], 'scooter': 'hello'},
         ['Invalid key(s): "bob", "scooter".']),
        ('PosixUIDGroupType', {'name_attr': 'user', 'member_attr': 'west', 'ldap_group_user_attr': 'legacyThing',
         'bob': ['a', 'b'], 'scooter': 'hello'},
         ['Invalid key(s): "bob", "member_attr", "scooter".']),
    ])
    def test_internal_value_invalid(self, group_type, data, expected):
        field = LDAPGroupTypeParamsField()
        field.get_depends_on = mock.MagicMock(return_value=group_type)

        with pytest.raises(ValidationError) as e:
            field.to_internal_value(data)
        assert e.value.detail == expected


class TestLDAPServerURIField():

    valid_URIs = [
        r'ldap://servername.so3:444',
        r'ldaps://servername3.s300:344'
    ]

    invalid_URIs = [
        r'ldaps://servername.-so3:444'
    ]

    def test_run_validators(self):
        field = LDAPServerURIField()
        for valid_uri in TestLDAPServerURIField.valid_URIs:
            assert field.run_validators(valid_uri) == valid_uri
        for invalid_uri in TestLDAPServerURIField.invalid_URIs:
            with pytest.raises(ValidationError):
                field.run_validators(invalid_uri)


Review comment (bug):
Could you use the pytest parameterization for these? like:

https://github.com/ansible/awx/blob/d9965cfe7e0f7a82fea04acb3b6b49852874f655/awx/main/tests/unit/utils/test_filters.py#L15

that helps to make it clear which case the test failed on if it fails.

Code after:
         ['Invalid key(s): "bob", "scooter".']),
        ('MemberDNGroupType', {'name_attr': 'user', 'member_attr': 'west', 'bob': ['a', 'b'], 'scooter': 'hello'},
         ['Invalid key(s): "bob", "scooter".']),
        ('PosixUIDGroupType', {'name_attr': 'user', 'member_attr': 'west', 'ldap_group_user_attr': 'legacyThing',
         'bob': ['a', 'b'], 'scooter': 'hello'},
         ['Invalid key(s): "bob", "member_attr", "scooter".']),
    ])
    def test_internal_value_invalid(self, group_type, data, expected):
        field = LDAPGroupTypeParamsField()
        field.get_depends_on = mock.MagicMock(return_value=group_type)

        with pytest.raises(ValidationError) as e:
            field.to_internal_value(data)
        assert e.value.detail == expected


class TestLDAPServerURIField():

    @pytest.mark.parametrize("ldap_uri, exception, expected", [
        (r'ldap://servername.com:444', None, r'ldap://servername.com:444'),
        (r'ldap://servername.so3:444', None, r'ldap://servername.so3:444'),
        (r'ldaps://servername3.s300:344', None, r'ldaps://servername3.s300:344'),
        (r'ldap://servername.-so3:444', ValidationError, None),
    ])
    def test_run_validators_valid(self, ldap_uri, exception, expected):
        field = LDAPServerURIField()
        if exception is None:
            assert field.run_validators(ldap_uri) == expected
        else:
            with pytest.raises(exception):
                field.run_validators(ldap_uri)