ronantakizawa/codereview-bench huggingface dataset:
Repository: jquery/jquery
File: test/unit/ajax.js (JavaScript)

Code before:
			done();
		} );
	} );

	// We need to simulate cross-domain requests with the feature that both 127.0.0.1 and localhost point to the mock http server
	// So skip the the test if we are not in localhost.
	QUnit[ ( !QUnit.isSwarm || location.hostname === "localhost" ) ? "test" : "skip" ](
		"jQuery.append with crossorigin attribute", function( assert ) {
		assert.expect( 1 );

		var done = assert.async(),
				timeout;

		Globals.register( "corsCallback" );
		window.corsCallback = function( response ) {
			assert.ok( response === true, "Callback fired" );
			window.clearTimeout( timeout );
			done();
		};

		var src = baseURL + "mock.php?action=cors";

		// Simulate a cross-origin request.
		// Full cross-origin verification is only done when tests run from localhost,
		// e.g. when run via Karma (as we can depend on localhost resolving to 127.0.0.1).
		// In other cases we only do a more limited check: that the Origin header is sent.
		src = src.replace( "localhost", "127.0.0.1" );
		var html = "<script type=\"text/javascript\" src=\"" + src + "\" crossorigin=\"anonymous\"><\/script>";

		jQuery( document.body ).append( html );
		timeout = window.setTimeout( function() {
			assert.ok( false, "Callback should have fired" );
			done();
		}, 2000 );
	} );

//----------- jQuery.active

	QUnit.test( "jQuery.active", function( assert ) {
		assert.expect( 1 );
		assert.ok( jQuery.active === 0, "ajax active counter should be zero: " + jQuery.active );
	} );

} )();


Review comment (suggestion):
There are no other cases anymore as this test is skipped outside of `localhost`. Perhaps this line should be removed.

Code after:
						assert.strictEqual( jQuery( "result", this ).text(), "3", "Check for XML" );
					} );
				}
			),
			jQuery.post( url( "mock.php?action=xml&cal=5-2" ), {}, function( xml ) {
				jQuery( "math", xml ).each( function() {
					assert.strictEqual( jQuery( "calculation", this ).text(), "5-2", "Check for XML" );
					assert.strictEqual( jQuery( "result", this ).text(), "3", "Check for XML" );
				} );
			} )
		).always( function() {
			done();
		} );
	} );

	QUnit.test( "jQuery[get|post]( options ) - simple with xml", function( assert ) {
		assert.expect( 2 );
		var done = assert.async();

		jQuery.when.apply( jQuery,
			jQuery.map( [ "get", "post" ], function( method ) {
				return jQuery[ method ]( {
					url: url( "mock.php?action=xml" ),
					data: {
						cal: "5-2"
					},
					success: function( xml ) {
						jQuery( "math", xml ).each( function() {
							assert.strictEqual( jQuery( "result", this ).text(), "3", "Check for XML" );
						} );
					}
				} );
			} )
		).always( function() {
			done();
		} );
	} );

//----------- jQuery.active

	QUnit.test( "jQuery.active", function( assert ) {
		assert.expect( 1 );
		assert.ok( jQuery.active === 0, "ajax active counter should be zero: " + jQuery.active );
	} );

} )();
Repository: jquery/jquery
File: src/manipulation/_evalUrl.js (JavaScript)

Code before:
import jQuery from "../ajax.js";

jQuery._evalUrl = function( url, options, doc ) {
	return jQuery.ajax( {
		url: url,

		// Make this explicit, since user can override this through ajaxSetup (#11264)
		type: "GET",
		dataType: "script",
		cache: true,
		async: false,
		global: false,
		scriptAttrs: options.crossOrigin ? { "crossOrigin": options.crossOrigin } : null,

		// Only evaluate the response if it is successful (gh-4126)
		// dataFilter is not invoked for failure responses, so using it instead
		// of the default converter is kludgy but it works.
		converters: {
			"text script": function() {}
		},
		dataFilter: function( response ) {
			jQuery.globalEval( response, options, doc );
		}
	} );
};


Review comment (suggestion):
This will save one byte:
```suggestion
		scriptAttrs: options.crossOrigin ? { "crossOrigin": options.crossOrigin } : undefined,
```

Code after:
import jQuery from "../ajax.js";

jQuery._evalUrl = function( url, options, doc ) {
	return jQuery.ajax( {
		url: url,

		// Make this explicit, since user can override this through ajaxSetup (#11264)
		type: "GET",
		dataType: "script",
		cache: true,
		async: false,
		global: false,
		scriptAttrs: options.crossOrigin ? { "crossOrigin": options.crossOrigin } : undefined,

		// Only evaluate the response if it is successful (gh-4126)
		// dataFilter is not invoked for failure responses, so using it instead
		// of the default converter is kludgy but it works.
		converters: {
			"text script": function() {}
		},
		dataFilter: function( response ) {
			jQuery.globalEval( response, options, doc );
		}
	} );
};
Repository: jquery/jquery
File: src/manipulation/_evalUrl.js (JavaScript)

Code before:
import jQuery from "../ajax.js";

jQuery._evalUrl = function( url, options, doc ) {
	return jQuery.ajax( {
		url: url,

		// Make this explicit, since user can override this through ajaxSetup (#11264)
		type: "GET",
		dataType: "script",
		cache: true,
		async: false,
		global: false,
		scriptAttrs: options.crossOrigin ? { "crossOrigin": options.crossOrigin } : null,

		// Only evaluate the response if it is successful (gh-4126)
		// dataFilter is not invoked for failure responses, so using it instead
		// of the default converter is kludgy but it works.
		converters: {
			"text script": function() {}
		},
		dataFilter: function( response ) {
			jQuery.globalEval( response, options, doc );
		}
	} );
};


Review comment (suggestion):
@jquery/core I'm starting to think this may be a breaking change because that forces scripts with the `crossorigin` attribute to use the script transport which is `async`. Should we perhaps support sync scripts there? Adding to the agenda for our next meeting.

Code after:
import jQuery from "../ajax.js";

jQuery._evalUrl = function( url, options, doc ) {
	return jQuery.ajax( {
		url: url,

		// Make this explicit, since user can override this through ajaxSetup (#11264)
		type: "GET",
		dataType: "script",
		cache: true,
		async: false,
		global: false,
		scriptAttrs: options.crossOrigin ? { "crossOrigin": options.crossOrigin } : undefined,

		// Only evaluate the response if it is successful (gh-4126)
		// dataFilter is not invoked for failure responses, so using it instead
		// of the default converter is kludgy but it works.
		converters: {
			"text script": function() {}
		},
		dataFilter: function( response ) {
			jQuery.globalEval( response, options, doc );
		}
	} );
};
Repository: jquery/jquery
File: test/unit/ajax.js (JavaScript)

Code before:
	QUnit[ ( !QUnit.isSwarm || location.hostname === "localhost" ) ? "test" : "skip" ](
		"jQuery.append with crossorigin attribute", function( assert ) {
		assert.expect( 1 );

		var done = assert.async(),
				timeout;

		Globals.register( "corsCallback" );
		window.corsCallback = function( response ) {
			assert.ok( response === true, "Callback fired" );
			window.clearTimeout( timeout );
			done();
		};

		var src = baseURL + "mock.php?action=cors";

		// Simulate a cross-origin request.
		// Full cross-origin verification is only done when tests run from localhost,
		// e.g. when run via Karma (as we can depend on localhost resolving to 127.0.0.1).
		// In other cases we only do a more limited check: that the Origin header is sent.
		src = src.replace( "localhost", "127.0.0.1" );
		var html = "<script type=\"text/javascript\" src=\"" + src + "\" crossorigin=\"anonymous\"><\/script>";

		jQuery( document.body ).append( html );
		timeout = window.setTimeout( function() {
			assert.ok( false, "Callback should have fired" );
			done();
		}, 2000 );
	} );

//----------- jQuery.active

	QUnit.test( "jQuery.active", function( assert ) {
		assert.expect( 1 );
		assert.ok( jQuery.active === 0, "ajax active counter should be zero: " + jQuery.active );
	} );

} )();


Review comment (suggestion):
```suggestion
			assert.ok( false, "Origin header should have been sent );
```

Code after:
					assert.strictEqual( jQuery( "calculation", this ).text(), "5-2", "Check for XML" );
					assert.strictEqual( jQuery( "result", this ).text(), "3", "Check for XML" );
				} );
			} )
		).always( function() {
			done();
		} );
	} );

	QUnit.test( "jQuery[get|post]( options ) - simple with xml", function( assert ) {
		assert.expect( 2 );
		var done = assert.async();

		jQuery.when.apply( jQuery,
			jQuery.map( [ "get", "post" ], function( method ) {
				return jQuery[ method ]( {
					url: url( "mock.php?action=xml" ),
					data: {
						cal: "5-2"
					},
					success: function( xml ) {
						jQuery( "math", xml ).each( function() {
							assert.strictEqual( jQuery( "result", this ).text(), "3", "Check for XML" );
						} );
					}
				} );
			} )
		).always( function() {
			done();
		} );
	} );

//----------- jQuery.active

	QUnit.test( "jQuery.active", function( assert ) {
		assert.expect( 1 );
		assert.ok( jQuery.active === 0, "ajax active counter should be zero: " + jQuery.active );
	} );

} )();
Repository: jquery/jquery
File: src/deprecated.js (JavaScript)

Code before:

		// ( namespace ) or ( selector, types [, fn] )
		return arguments.length === 1 ?
			this.off( selector, "**" ) :
			this.off( types, selector || "**", fn );
	},

	hover: function( fnOver, fnOut ) {
		return this.mouseenter( fnOver ).mouseleave( fnOut || fnOver );
	}
} );

jQuery.each( ( "blur focus focusin focusout resize scroll click dblclick " +
	"mousedown mouseup mousemove mouseover mouseout mouseenter mouseleave " +
	"change select submit keydown keypress keyup contextmenu" ).split( " " ),
	function( _i, name ) {

		// Handle event binding
		jQuery.fn[ name ] = function( data, fn ) {
			return arguments.length > 0 ?
				this.on( name, null, data, fn ) :
				this.trigger( name );
		};
	} );

jQuery.each( [
	"ajaxStart",
	"ajaxStop",
	"ajaxComplete",
	"ajaxError",
	"ajaxSuccess",
	"ajaxSend"
], function( _i, type ) {
	jQuery.fn[ type ] = function( fn ) {
		return this.on( type, fn );
	};
} );

// Bind a function to a context, optionally partially applying any
// arguments.
// jQuery.proxy is deprecated to promote standards (specifically Function#bind)
// However, it is not slated for removal any time soon
jQuery.proxy = function( fn, context ) {
	var tmp, args, proxy;

	if ( typeof context === "string" ) {
		tmp = fn[ context ];
		context = fn;
		fn = tmp;
	}


Review comment (suggestion):
~~I think I need to wrap it with an `if ( jQuery.ajax )` condition to better support custom builds.~~ (see the next comment)

Code after:
	// Quick check to determine if target is callable, in the spec
	// this throws a TypeError, but we will just return undefined.
	if ( typeof fn !== "function" ) {
		return undefined;
	}

	// Simulated bind
	args = slice.call( arguments, 2 );
	proxy = function() {
		return fn.apply( context || this, args.concat( slice.call( arguments ) ) );
	};

	// Set the guid of unique handler to the same of original handler, so it can be removed
	proxy.guid = fn.guid = fn.guid || jQuery.guid++;

	return proxy;
};

jQuery.holdReady = function( hold ) {
	if ( hold ) {
		jQuery.readyWait++;
	} else {
		jQuery.ready( true );
	}
};

jQuery.trim = function( text ) {
	return text == null ? "" : trim.call( text );
};
Repository: jquery/jquery
File: build/tasks/build.js (JavaScript)

Code before:
			done( err );
		}
	} );

	// Special "alias" task to make custom build creation less grawlix-y
	// Translation example
	//
	//   grunt custom:+ajax,-dimensions,-effects,-offset
	//
	// Becomes:
	//
	//   grunt build:*:*:+ajax:-dimensions:-effects:-offset
	grunt.registerTask( "custom", function() {
		const args = this.args;
		const modules = args.length ? args[ 0 ].replace( /,/g, ":" ) : "";
		const done = this.async();
		const insight = new Insight( {
			trackingCode: "UA-1076265-4",
			pkg: pkg
		} );

		function exec( trackingAllowed ) {
			let tracks = args.length ? args[ 0 ].split( "," ) : [];
			const defaultPath = [ "build", "custom" ];

			tracks = tracks.map( function( track ) {
				return track.replace( /\//g, "+" );
			} );

			if ( trackingAllowed ) {

				// Track individuals
				tracks.forEach( function( module ) {
					const path = defaultPath.concat( [ "individual" ], module );

					insight.track.apply( insight, path );
				} );

				// Track full command
				insight.track.apply( insight, defaultPath.concat( [ "full" ], tracks ) );
			}

			grunt.task.run( [ "build:*:*" + ( modules ? ":" + modules : "" ), "uglify", "dist" ] );
			done();
		}

		grunt.log.writeln( "Creating custom build...\n" );

		// Ask for permission the first time
		if ( insight.optOut === undefined ) {
			insight.askPermission( null, function( _error, result ) {

Review comment (suggestion):
`flatMap` would be so useful here... Alas, it's Node 12+ and we'll have to support at least v10 for the time being.

Code after:
			done( err );
		}
	} );

	// Special "alias" task to make custom build creation less grawlix-y
	// Translation example
	//
	//   grunt custom:+ajax,-dimensions,-effects,-offset
	//
	// Becomes:
	//
	//   grunt build:*:*:+ajax:-dimensions:-effects:-offset
	//
	// There's also a special "slim" alias that resolves to the jQuery Slim build
	// configuration:
	//
	//   grunt custom:slim
	grunt.registerTask( "custom", function() {
		const args = this.args;
		const modules = args.length ?
			args[ 0 ]
				.split( "," )

				// Replace "slim" with respective exclusions meant for
				// the official slim build
				.reduce( ( acc, elem ) => acc.concat(
					elem === "slim" ?
						[ "-ajax", "-effects" ] :
						[ elem ]
				), [] )

				.join( ":" ) :
			"";
		const done = this.async();
		const insight = new Insight( {
			trackingCode: "UA-1076265-4",
			pkg: pkg
		} );

		function exec( trackingAllowed ) {
			let tracks = args.length ? args[ 0 ].split( "," ) : [];
			const defaultPath = [ "build", "custom" ];

			tracks = tracks.map( function( track ) {
				return track.replace( /\//g, "+" );
			} );

			if ( trackingAllowed ) {

				// Track individuals
				tracks.forEach( function( module ) {
Repository: jquery/jquery
File: test/unit/data.js (JavaScript)

Code before:
	assert.expect( 1 );

	var key,
		div = jQuery( "<div/>" );

	div.data( "foo" );
	assert.equal( jQuery.hasData( div[ 0 ] ), false, "No data exists after access" );

	// Make sure no expando has been added
	for ( key in div[ 0 ] ) {
		if ( /^jQuery/.test( key ) ) {
			assert.ok( false, "Expando was created on access" );
		}
	}
} );

QUnit.test( "keys matching Object.prototype properties  (gh-3256)", function( assert ) {
	assert.expect( 2 );

	var div = jQuery( "<div/>" );

	assert.strictEqual( div.data( "hasOwnProperty" ), undefined,
		"hasOwnProperty not matched (before forced data creation)" );

	// Query the collection to force the creation of a data object for this element.
	div.data();

	assert.strictEqual( div.data( "hasOwnProperty" ), undefined,
		"hasOwnProperty not matched (after forced data creation)" );
} );


Review comment (suggestion):
I think this should be even stronger.
```suggestion
	div.data( { foo: "bar" } );
```

Code after:
	assert.expect( 1 );

	var key,
		div = jQuery( "<div/>" );

	div.data( "foo" );
	assert.equal( jQuery.hasData( div[ 0 ] ), false, "No data exists after access" );

	// Make sure no expando has been added
	for ( key in div[ 0 ] ) {
		if ( /^jQuery/.test( key ) ) {
			assert.ok( false, "Expando was created on access" );
		}
	}
} );

QUnit.test( "keys matching Object.prototype properties  (gh-3256)", function( assert ) {
	assert.expect( 2 );

	var div = jQuery( "<div/>" );

	assert.strictEqual( div.data( "hasOwnProperty" ), undefined,
		"hasOwnProperty not matched (before forced data creation)" );

	// Force the creation of a data object for this element.
	div.data( { foo: "bar" } );

	assert.strictEqual( div.data( "hasOwnProperty" ), undefined,
		"hasOwnProperty not matched (after forced data creation)" );
} );
Repository: jquery/jquery
File: src/ajax.js (JavaScript)

Code before:
	}
} );

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 ) {
	if ( s.data && s.processData ) {
		for ( var i in s.headers ) {
			if ( i.toLowerCase() === "content-type" ) {
				s.contentType = s.headers[ i ] || "";
			}
		}
	}
} );

export default jQuery;


Review comment (suggestion):
I don't think this check is needed. The value of `contentType` shouldn't depend on whether data is passed & being processed.

Code after:
	}
} );

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;
Repository: jquery/jquery
File: src/ajax.js (JavaScript)

Code before:
	}
} );

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 ) {
	for ( var i in s.headers ) {
		if ( i.toLowerCase() === "content-type" ) {
			s.contentType = s.headers[ i ] || "";
		}
	}
} );

export default jQuery;


Review comment (suggestion):
Our code style requires separate variable declarations at the beginning of the function. So:
```suggestion
	var i;
	for ( i in s.headers ) {
```

Code after:
	}
} );

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;
Repository: jquery/jquery
File: test/unit/ajax.js (JavaScript)

Code before:
		};
	} );

	ajaxTest( "jQuery.ajax() - escape %20 with contentType override (gh-4119)", 1, function( assert ) {
		return {
			url: "bogus.html",
			contentType: "application/json",
			headers: { "content-type": "application/x-www-form-urlencoded" },
			method: "post",
			dataType: "json",
			data: "{\"val\":\"%20\"}",
			beforeSend: function( _, s ) {
				assert.strictEqual( s.data, "{\"val\":\"+\"}", "data is %20-encoded" );
				return false;
			},
			error: true
		};
	} );

	ajaxTest( "jQuery.ajax() - override contentType with header (gh-4119)", 1, function( assert ) {
		return {
			url: "bogus.html",
			contentType: "application/json",
			headers: { "content-type": "application/x-www-form-urlencoded" },
			beforeSend: function( _, s ) {
				assert.strictEqual( s.contentType, "application/x-www-form-urlencoded", "contentType is overwritten" );
				return false;
			},
			error: true
		};
	} );

	ajaxTest( "jQuery.ajax() - data - no processing POST", 1, function( assert ) {
		return {
			url: "bogus.html",
			data: { devo: "A Beautiful World" },
			type: "post",
			contentType: "x-special-sauce",
			processData: false,
			beforeSend: function( _, s ) {
				assert.deepEqual( s.data, { devo: "A Beautiful World" }, "data is not processed" );
				return false;
			},
			error: true
		};
	} );

	ajaxTest( "jQuery.ajax() - data - no processing GET", 1, function( assert ) {
		return {
			url: "bogus.html",
			data: { devo: "A Beautiful World" },

Review comment (suggestion):
This line is quite long, could you break it?
```suggestion
				assert.strictEqual( s.contentType, "application/x-www-form-urlencoded",
					"contentType is overwritten" );
```

Code after:
		};
	} );

	ajaxTest( "jQuery.ajax() - escape %20 with contentType override (gh-4119)", 1, function( assert ) {
		return {
			url: "bogus.html",
			contentType: "application/json",
			headers: { "content-type": "application/x-www-form-urlencoded" },
			method: "post",
			dataType: "json",
			data: "{\"val\":\"%20\"}",
			beforeSend: function( _, s ) {
				assert.strictEqual( s.data, "{\"val\":\"+\"}", "data is %20-encoded" );
				return false;
			},
			error: true
		};
	} );

	ajaxTest( "jQuery.ajax() - override contentType with header (gh-4119)", 1, function( assert ) {
		return {
			url: "bogus.html",
			contentType: "application/json",
			headers: { "content-type": "application/x-www-form-urlencoded" },
			beforeSend: function( _, s ) {
				assert.strictEqual( s.contentType, "application/x-www-form-urlencoded",
					"contentType is overwritten" );
				return false;
			},
			error: true
		};
	} );

	ajaxTest( "jQuery.ajax() - data - no processing POST", 1, function( assert ) {
		return {
			url: "bogus.html",
			data: { devo: "A Beautiful World" },
			type: "post",
			contentType: "x-special-sauce",
			processData: false,
			beforeSend: function( _, s ) {
				assert.deepEqual( s.data, { devo: "A Beautiful World" }, "data is not processed" );
				return false;
			},
			error: true
		};
	} );

	ajaxTest( "jQuery.ajax() - data - no processing GET", 1, function( assert ) {
		return {
			url: "bogus.html",