ronantakizawa/codereview-bench huggingface dataset:
Repository: jquery/jquery
File: src/manipulation/buildFragment.js (JavaScript)

Code before:
	for ( ; i < l; i++ ) {
		elem = elems[ i ];

		if ( elem || elem === 0 ) {

			// Add nodes directly
			if ( toType( elem ) === "object" ) {
				jQuery.merge( nodes, elem.nodeType ? [ elem ] : elem );

			// Convert non-html into a text node
			} else if ( !rhtml.test( elem ) ) {
				nodes.push( context.createTextNode( elem ) );

			// Convert html into DOM nodes
			} else {
				tmp = tmp || fragment.appendChild( context.createElement( "div" ) );

				// Deserialize a standard representation
				tag = ( rtagName.exec( elem ) || [ "", "" ] )[ 1 ].toLowerCase();
				wrap = wrapMap[ tag ] || arr;

				// Create wrappers & descend into them.
				j = wrap.length;
				while ( --j > -1 ) {
					tmp.appendChild( document.createElement( wrap[ j ] ) );
					tmp = tmp.firstChild;
				}

				tmp.innerHTML = jQuery.htmlPrefilter( elem );

				jQuery.merge( nodes, tmp.childNodes );

				// Remember the top-level container
				tmp = fragment.firstChild;

				// Ensure the created nodes are orphaned (#12392)
				tmp.textContent = "";
			}
		}
	}

	// Remove wrapper from fragment
	fragment.textContent = "";

	i = 0;
	while ( ( elem = nodes[ i++ ] ) ) {

		// Skip elements already in the context collection (trac-4087)
		if ( selection && jQuery.inArray( elem, selection ) > -1 ) {
			if ( ignored ) {
				ignored.push( elem );

Review comment (suggestion):
I think this needs to use `context` rather than `document`, and will probably be smaller as one statement rather than two.
```suggestion
					tmp = tmp.appendChild( context.createElement( wrap[ j ] ) );
```

Code after:
		elem = elems[ i ];

		if ( elem || elem === 0 ) {

			// Add nodes directly
			if ( toType( elem ) === "object" ) {
				jQuery.merge( nodes, elem.nodeType ? [ elem ] : elem );

			// Convert non-html into a text node
			} else if ( !rhtml.test( elem ) ) {
				nodes.push( context.createTextNode( elem ) );

			// Convert html into DOM nodes
			} else {
				tmp = tmp || fragment.appendChild( context.createElement( "div" ) );

				// Deserialize a standard representation
				tag = ( rtagName.exec( elem ) || [ "", "" ] )[ 1 ].toLowerCase();
				wrap = wrapMap[ tag ] || arr;

				// Create wrappers & descend into them.
				j = wrap.length;
				while ( --j > -1 ) {
					tmp = tmp.appendChild( context.createElement( wrap[ j ] ) );
				}

				tmp.innerHTML = jQuery.htmlPrefilter( elem );

				jQuery.merge( nodes, tmp.childNodes );

				// Remember the top-level container
				tmp = fragment.firstChild;

				// Ensure the created nodes are orphaned (#12392)
				tmp.textContent = "";
			}
		}
	}

	// Remove wrapper from fragment
	fragment.textContent = "";

	i = 0;
	while ( ( elem = nodes[ i++ ] ) ) {

		// Skip elements already in the context collection (trac-4087)
		if ( selection && jQuery.inArray( elem, selection ) > -1 ) {
			if ( ignored ) {
				ignored.push( elem );
			}
			continue;
Repository: jquery/jquery
File: src/ajax/script.js (JavaScript)

Code before:
// Install script dataType
jQuery.ajaxSetup( {
	accepts: {
		script: "text/javascript, application/javascript, " +
			"application/ecmascript, application/x-ecmascript"
	},
	contents: {
		script: /\b(?:java|ecma)script\b/
	},
	converters: {
		"text script": function( text ) {
			jQuery.globalEval( text );
			return text;
		}
	}
} );

// Handle cache's special case and crossDomain
jQuery.ajaxPrefilter( "script", function( s ) {
	if ( s.cache === undefined ) {
		s.cache = false;
	}

	// These types of requests are handled via a script tag
	// so force their methods to GET.
	if ( s.async || s.crossDomain || s.scriptAttrs ) {
		s.type = "GET";
	}
} );

// Bind script tag hack transport
jQuery.ajaxTransport( "script", function( s ) {

	// This transport only deals with async, cross domain or forced-by-attrs requests.
	// Sync requests remain handled differently to preserve strict script ordering.
	if ( s.async || s.crossDomain || s.scriptAttrs ) {
		var script, callback;
		return {
			send: function( _, complete ) {
				script = jQuery( "<script>" )
					.attr( s.scriptAttrs || {} )
					.prop( { charset: s.scriptCharset, src: s.url } )
					.on( "load error", callback = function( evt ) {
						script.remove();
						callback = null;
						if ( evt ) {
							complete( evt.type === "error" ? 404 : 200, evt.type );
						}
					} );

				// Use native DOM manipulation to avoid our domManip AJAX trickery

Review comment (performance):
Did you optimize this order for gzip compression? If not, I suspect placing `s.crossDomain` first might chop off a byte or two.

Code after:
// Install script dataType
jQuery.ajaxSetup( {
	accepts: {
		script: "text/javascript, application/javascript, " +
			"application/ecmascript, application/x-ecmascript"
	},
	contents: {
		script: /\b(?:java|ecma)script\b/
	},
	converters: {
		"text script": function( text ) {
			jQuery.globalEval( text );
			return text;
		}
	}
} );

// Handle cache's special case and crossDomain
jQuery.ajaxPrefilter( "script", function( s ) {
	if ( s.cache === undefined ) {
		s.cache = false;
	}

	// These types of requests are handled via a script tag
	// so force their methods to GET.
	if ( s.crossDomain || s.async || s.scriptAttrs ) {
		s.type = "GET";
	}
} );

// Bind script tag hack transport
jQuery.ajaxTransport( "script", function( s ) {

	// This transport only deals with async, cross domain or forced-by-attrs requests.
	// Sync requests remain handled differently to preserve strict script ordering.
	if ( s.crossDomain || s.async || s.scriptAttrs ) {
		var script, callback;
		return {
			send: function( _, complete ) {
				script = jQuery( "<script>" )
					.attr( s.scriptAttrs || {} )
					.prop( { charset: s.scriptCharset, src: s.url } )
					.on( "load error", callback = function( evt ) {
						script.remove();
						callback = null;
						if ( evt ) {
							complete( evt.type === "error" ? 404 : 200, evt.type );
						}
					} );

				// Use native DOM manipulation to avoid our domManip AJAX trickery
Repository: jquery/jquery
File: test/unit/ajax.js (JavaScript)

Code before:
				assert.ok( true, "Converter for unsuccessful response" );
			}
		};

		function request( title, options ) {
			var testMsg = title + ": expected JSON response body";
			return jQuery.extend( {
				beforeSend: function() {
					jQuery.globalEval = function() {
						assert.ok( true, "Should eval" );
					};
				},
				complete: function() {
					jQuery.globalEval = globalEval;
				},
				// error is the significant assertion
				error: function( xhr ) {
					assert.strictEqual( xhr.responseBody, { id: 1 }, testMsg );
				},
				success: function() {
					assert.ok( false, "Unanticipated success" );
				}
			}, options );
		}
		return [
			request(
				"JSONP reply with dataType",
				{
					dataType: "jsonp",
					url: url( "mock.php?action=errorWithJSONPAndResponseBodyScript" )
				}
			)
		];
	} );

	ajaxTest( "jQuery.ajax() - do not execute scripts from unsuccessful responses (gh-4250)", 11, function( assert ) {
		var globalEval = jQuery.globalEval;

		var failConverters = {
			"text script": function() {
				assert.ok( false, "No converter for unsuccessful response" );
			}
		};

		function request( title, options ) {
			var testMsg = title + ": expected file missing status";
			return jQuery.extend( {
				beforeSend: function() {
					jQuery.globalEval = function() {
						assert.ok( false, "Should not eval" );
					};

Review comment (suggestion):
In this case, where you only have one request, wrapping it in a `request` function doesn't really help much. Also, you can just return an object, `ajaxTest` wraps it in a one-element array by itself. You can see many other AJAX tests are structured like that.

Code after:
			// error is the significant assertion
			error: function( xhr ) {
				var expected = { "status": 404, "msg": "Not Found" };
				assert.deepEqual( xhr.responseJSON, expected, testMsg );
			}
		};
	} );

	ajaxTest( "jQuery.ajax() - do not execute scripts from unsuccessful responses (gh-4250)", 11, function( assert ) {
		var globalEval = jQuery.globalEval;

		var failConverters = {
			"text script": function() {
				assert.ok( false, "No converter for unsuccessful response" );
			}
		};

		function request( title, options ) {
			var testMsg = title + ": expected file missing status";
			return jQuery.extend( {
				beforeSend: function() {
					jQuery.globalEval = function() {
						assert.ok( false, "Should not eval" );
					};
				},
				complete: function() {
					jQuery.globalEval = globalEval;
				},
				// error is the significant assertion
				error: function( xhr ) {
					assert.strictEqual( xhr.status, 404, testMsg );
				},
				success: function() {
					assert.ok( false, "Unanticipated success" );
				}
			}, options );
		}

		return [
			request(
				"HTML reply",
				{
					url: url( "404.txt" )
				}
			),
			request(
				"HTML reply with dataType",
				{
					dataType: "script",
					url: url( "404.txt" )
				}
Repository: jquery/jquery
File: src/css/support.js (JavaScript)

Code before:
import jQuery from "../core.js";
import document from "../var/document.js";
import documentElement from "../var/documentElement.js";
import support from "../var/support.js";

( function() {

var reliableTrDimensionsVal,
	div = document.createElement( "div" );

// Finish early in limited (non-browser) environments
if ( !div.style ) {
	return;
}

jQuery.extend( support, {

	// Support: Firefox 70+
	// Only Firefox includes border widths
	// in computed dimensions. (gh-4529)
	reliableTrDimensions: function() {
		var table, tr, trStyle;
		if ( reliableTrDimensionsVal == null ) {
			table = document.createElement( "table" );
			tr = document.createElement( "tr" );

			table.style.cssText = "position:absolute;left:-11111px;border-collapse:separate";
			tr.style.cssText = "border:1px solid";

			// Support: Chrome 86+
			// Height set through cssText does not get applied.
			// Computed height then comes back as 0.
			tr.style.height = "1px";
			div.style.height = "9px";

			documentElement
				.appendChild( table )

Review comment (bug):
Reaching here will make `support.reliableTrDimensions()` crash. I guess this is similar to what we do on `3.x-stable`...

Code after:
import document from "../var/document.js";
import documentElement from "../var/documentElement.js";
import support from "../var/support.js";

( function() {

var reliableTrDimensionsVal,
	div = document.createElement( "div" );

// Finish early in limited (non-browser) environments
if ( !div.style ) {
	return;
}

// Support: IE 10 - 11+
// IE misreports `getComputedStyle` of table rows with width/height
// set in CSS while `offset*` properties report correct values.
// Support: Firefox 70+
// Only Firefox includes border widths
// in computed dimensions. (gh-4529)
support.reliableTrDimensions = function() {
	var table, tr, trStyle;
	if ( reliableTrDimensionsVal == null ) {
		table = document.createElement( "table" );
		tr = document.createElement( "tr" );

		table.style.cssText = "position:absolute;left:-11111px;border-collapse:separate";
		tr.style.cssText = "border:1px solid";

		// Support: Chrome 86+
		// Height set through cssText does not get applied.
		// Computed height then comes back as 0.
		tr.style.height = "1px";
		div.style.height = "9px";

		documentElement
			.appendChild( table )
Repository: jquery/jquery
File: src/css/curCSS.js (JavaScript)

Code before:
import jQuery from "../core.js";
import isAttached from "../core/isAttached.js";
import getStyles from "./var/getStyles.js";

function curCSS( elem, name, computed ) {
	var ret;

	computed = computed || getStyles( elem );

	// getPropertyValue is needed for `.css('--customProperty')` (gh-3144)
	if ( computed ) {
		ret = computed.getPropertyValue( name );

		// trim whitespace (issue #4926)
		ret = ( ret && ret.trim() ) || computed[ name ];

		if ( ret === "" && !isAttached( elem ) ) {
			ret = jQuery.style( elem, name );
		}
	}

	return ret !== undefined ?

		// Support: IE <=9 - 11+
		// IE returns zIndex value as an integer.
		ret + "" :
		ret;
}

export default curCSS;


Review comment (suggestion):
This trims all values. While this generally happens for regular properties, it seems safer to just trim where needed, i.e. for custom properties.

We have the `rcustomProp` regex defined in `css.js`; let's move it to a separate file `css/var/rcustomProp.js` (you can see other files in `var` for a pattern how it's used) and use both here and in `css.js` to detect custom properties and only trim in that case.

Code after:
import jQuery from "../core.js";
import isAttached from "../core/isAttached.js";
import getStyles from "./var/getStyles.js";
import rcustomProp from "./var/rcustomProp.js";
import rtrim from "../var/rtrim.js";

function curCSS( elem, name, computed ) {
	var ret,
		isCustomProp = rcustomProp.test( name );

	computed = computed || getStyles( elem );

	// getPropertyValue is needed for `.css('--customProperty')` (gh-3144)
	if ( computed ) {
		ret = computed.getPropertyValue( name ) || computed[ name ];

		// trim whitespace for custom property (issue gh-4926)
		if ( isCustomProp ) {
			ret = ret.replace( rtrim, "$1" );
		}

		if ( ret === "" && !isAttached( elem ) ) {
			ret = jQuery.style( elem, name );
		}
	}

	return ret !== undefined ?

		// Support: IE <=9 - 11+
		// IE returns zIndex value as an integer.
		ret + "" :
		ret;
}

export default curCSS;
Repository: jquery/jquery
File: src/css/curCSS.js (JavaScript)

Code before:
import jQuery from "../core.js";
import isAttached from "../core/isAttached.js";
import getStyles from "./var/getStyles.js";
import rcustomProp from "./var/rcustomProp.js";
import rtrim from "../var/rtrim.js";

function curCSS( elem, name, computed ) {
	var ret,
		isCustomProp = rcustomProp.test( name );

	computed = computed || getStyles( elem );

	// getPropertyValue is needed for `.css('--customProperty')` (gh-3144)
	if ( computed ) {
		ret = computed.getPropertyValue( name ) || computed[ name ];

		// trim whitespace for custom property (issue #4926)
		if ( isCustomProp ) {
			ret = ret.replace( rtrim, "$1" );
		}

		if ( ret === "" && !isAttached( elem ) ) {
			ret = jQuery.style( elem, name );
		}
	}

	return ret !== undefined ?

		// Support: IE <=9 - 11+
		// IE returns zIndex value as an integer.
		ret + "" :
		ret;
}

export default curCSS;


Review comment (bug):
That's a jQuery-specific gotcha:
```suggestion
		// trim whitespace for custom property (issue gh-4926)
```
Since we have lots of references using `#` to our legacy Trac bug tracker at https://bugs.jquery.com/, we use the `gh-XXXX` syntax to make it explicit what comes from GitHub.

We'll need to replace these legacy `#` uses one day to reduce confusion.

Code after:
import jQuery from "../core.js";
import isAttached from "../core/isAttached.js";
import getStyles from "./var/getStyles.js";
import rcustomProp from "./var/rcustomProp.js";
import rtrim from "../var/rtrim.js";

function curCSS( elem, name, computed ) {
	var ret,
		isCustomProp = rcustomProp.test( name );

	computed = computed || getStyles( elem );

	// getPropertyValue is needed for `.css('--customProperty')` (gh-3144)
	if ( computed ) {
		ret = computed.getPropertyValue( name ) || computed[ name ];

		// trim whitespace for custom property (issue gh-4926)
		if ( isCustomProp ) {
			ret = ret.replace( rtrim, "$1" );
		}

		if ( ret === "" && !isAttached( elem ) ) {
			ret = jQuery.style( elem, name );
		}
	}

	return ret !== undefined ?

		// Support: IE <=9 - 11+
		// IE returns zIndex value as an integer.
		ret + "" :
		ret;
}

export default curCSS;
Repository: jquery/jquery
File: src/css/curCSS.js (JavaScript)

Code before:
import jQuery from "../core.js";
import isAttached from "../core/isAttached.js";
import getStyles from "./var/getStyles.js";

function curCSS( elem, name, computed ) {
	var ret;

	computed = computed || getStyles( elem );

	// getPropertyValue is needed for `.css('--customProperty')` (gh-3144)
	if ( computed ) {
		ret = computed.getPropertyValue( name ) || computed[ name ];

		if ( ret === "" && !isAttached( elem ) ) {
			ret = jQuery.style( elem, name );
		}
	}

	return ret !== undefined ?

		// Support: IE <=9 - 11+
		// IE returns zIndex value as an integer.
		ret + "" :
		ret;
}

export default curCSS;


Review comment (suggestion):
This throws `TypeError: ret is undefined` when the property isn’t set: #5105.

Code after:
import jQuery from "../core.js";
import isAttached from "../core/isAttached.js";
import getStyles from "./var/getStyles.js";
import rcustomProp from "./var/rcustomProp.js";
import rtrim from "../var/rtrim.js";

function curCSS( elem, name, computed ) {
	var ret,
		isCustomProp = rcustomProp.test( name );

	computed = computed || getStyles( elem );

	// getPropertyValue is needed for `.css('--customProperty')` (gh-3144)
	if ( computed ) {
		ret = computed.getPropertyValue( name ) || computed[ name ];

		// trim whitespace for custom property (issue gh-4926)
		if ( isCustomProp ) {
			ret = ret.replace( rtrim, "$1" );
		}

		if ( ret === "" && !isAttached( elem ) ) {
			ret = jQuery.style( elem, name );
		}
	}

	return ret !== undefined ?

		// Support: IE <=9 - 11+
		// IE returns zIndex value as an integer.
		ret + "" :
		ret;
}

export default curCSS;
Repository: jquery/jquery
File: Gruntfile.js (JavaScript)

Code before:
	grunt.registerTask( "lint", [
		"jsonlint",

		// Running the full eslint task without breaking it down to targets
		// would run the dist target first which would point to errors in the built
		// file, making it harder to fix them. We want to check the built file only
		// if we already know the source files pass the linter.
		runIfNewNode( "eslint:dev" ),
		runIfNewNode( "eslint:dist" )
	] );

	grunt.registerTask( "lint:newer", [
		"newer:jsonlint",

		// Don't replace it with just the task; see the above comment.
		runIfNewNode( "newer:eslint:dev" ),
		runIfNewNode( "newer:eslint:dist" )
	] );

	grunt.registerTask( "test:fast", runIfNewNode( "node_smoke_tests" ) );
	grunt.registerTask( "test:slow", [
		runIfNewNode( "promises_aplus_tests" ),

		// Support: Node.js 17+
		// jsdom fails to connect in Node 17. Until we figure out a fix, skip
		// jsdom tests there.
		nodeV17OrNewer ? "print_jsdom_message" : runIfNewNode( "karma:jsdom" )
	] );

	grunt.registerTask( "test:prepare", [
		"npmcopy",
		"qunit_fixture",
		"babel:tests"
	] );

	grunt.registerTask( "test", [
		"test:prepare",
		"test:fast",
		"test:slow"
	] );

	grunt.registerTask( "dev", [
		"build:*:*",
		runIfNewNode( "newer:eslint:dev" ),
		"newer:uglify",
		"remove_map_comment",
		"dist:*",
		"qunit_fixture",
		"compare_size"
	] );


Review comment (bug):
```suggestion
		// jsdom fails to connect to the Karma server in Node 17+.
		// Until we figure out a fix, skip jsdom tests there.
```

Code after:
	grunt.registerTask( "lint", [
		"jsonlint",

		// Running the full eslint task without breaking it down to targets
		// would run the dist target first which would point to errors in the built
		// file, making it harder to fix them. We want to check the built file only
		// if we already know the source files pass the linter.
		runIfNewNode( "eslint:dev" ),
		runIfNewNode( "eslint:dist" )
	] );

	grunt.registerTask( "lint:newer", [
		"newer:jsonlint",

		// Don't replace it with just the task; see the above comment.
		runIfNewNode( "newer:eslint:dev" ),
		runIfNewNode( "newer:eslint:dist" )
	] );

	grunt.registerTask( "test:fast", runIfNewNode( "node_smoke_tests" ) );
	grunt.registerTask( "test:slow", [
		runIfNewNode( "promises_aplus_tests" ),

		// Support: Node.js 17+
		// jsdom fails to connect to the Karma server in Node 17+.
		// Until we figure out a fix, skip jsdom tests there.
		nodeV17OrNewer ? "print_jsdom_message" : runIfNewNode( "karma:jsdom" )
	] );

	grunt.registerTask( "test:prepare", [
		"npmcopy",
		"qunit_fixture",
		"babel:tests"
	] );

	grunt.registerTask( "test", [
		"test:prepare",
		"test:fast",
		"test:slow"
	] );

	grunt.registerTask( "dev", [
		"build:*:*",
		runIfNewNode( "newer:eslint:dev" ),
		"newer:uglify",
		"remove_map_comment",
		"dist:*",
		"qunit_fixture",
		"compare_size"
	] );
Repository: jquery/jquery
File: src/ajax.js (JavaScript)

Code before:

				// Handle the global AJAX counter
				if ( !( --jQuery.active ) ) {
					jQuery.event.trigger( "ajaxStop" );
				}
			}
		}

		return jqXHR;
	},

	getJSON: function( url, data, callback ) {
		return jQuery.get( url, data, callback, "json" );
	},

	getScript: function( url, callback ) {
		return jQuery.get( url, undefined, callback, "script" );
	}
} );

jQuery.each( [ "get", "post" ], function( _i, method ) {
	jQuery[ method ] = function( url, data, callback, type ) {

		// Shift arguments if data argument was omitted
		if ( typeof data === "function" ) {
			type = type || callback;
			callback = data;
			data = undefined;
		}

		// The url can be an options object (which then must have .url)
		return jQuery.ajax( jQuery.extend( {
			url: url,
			type: method,
			dataType: type,
			data: data,
			success: callback
		}, jQuery.isPlainObject( url ) && url ) );
	};
} );

jQuery.ajaxPrefilter( function( s ) {
	var i;
	for ( i in s.headers ) {
		if ( i.toLowerCase() === "content-type" ) {
			s.contentType = s.headers[ i ] || "";
		}
	}
} );

export default jQuery;

Review comment (suggestion):
@gibson042 allowing `undefined` here would break the 4-param signature with the second parameter (`data`) being `undefined` which is e.g. used in the `$.getScript` definition. We could add more checks to make it work but I thought in that case it may be better to just allow `null`. `null` is not an allowed `data` value so we don’t have a conflict.

Code after:

				// Handle the global AJAX counter
				if ( !( --jQuery.active ) ) {
					jQuery.event.trigger( "ajaxStop" );
				}
			}
		}

		return jqXHR;
	},

	getJSON: function( url, data, callback ) {
		return jQuery.get( url, data, callback, "json" );
	},

	getScript: function( url, callback ) {
		return jQuery.get( url, undefined, callback, "script" );
	}
} );

jQuery.each( [ "get", "post" ], function( _i, method ) {
	jQuery[ method ] = function( url, data, callback, type ) {

		// Shift arguments if data argument was omitted.
		// Handle the null callback placeholder.
		if ( typeof data === "function" || data === null ) {
			type = type || callback;
			callback = data;
			data = undefined;
		}

		// The url can be an options object (which then must have .url)
		return jQuery.ajax( jQuery.extend( {
			url: url,
			type: method,
			dataType: type,
			data: data,
			success: callback
		}, jQuery.isPlainObject( url ) && url ) );
	};
} );

jQuery.ajaxPrefilter( function( s ) {
	var i;
	for ( i in s.headers ) {
		if ( i.toLowerCase() === "content-type" ) {
			s.contentType = s.headers[ i ] || "";
		}
	}
} );