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

Code before:
Tween.prototype = {
	constructor: Tween,
	init: function( elem, options, prop, end, easing, unit ) {
		this.elem = elem;
		this.prop = prop;
		this.easing = easing || jQuery.easing._default;
		this.options = options;
		this.start = this.now = this.cur();
		this.end = end;
		this.unit = unit || ( jQuery.cssNumber[ prop ] ? "" : "px" );
	},
	cur: function() {
		var hooks = Tween.propHooks[ this.prop ];

		return hooks && hooks.get ?
			hooks.get( this ) :
			Tween.propHooks._default.get( this );
	},
	run: function( percent ) {
		var hooks = Tween.propHooks[ this.prop ];

		this.pos = this.options.duration ?
			jQuery.easing[ this.easing ]( percent ) :
			percent;
		this.now = ( this.end - this.start ) * this.pos + this.start;

		if ( this.options.step ) {
			this.options.step.call( this.elem, this.now, this );
		}

		if ( hooks && hooks.set ) {
			hooks.set( this );
		} else {
			Tween.propHooks._default.set( this );
		}
		return this;
	}
};

Tween.prototype.init.prototype = Tween.prototype;

Tween.propHooks = {
	_default: {
		get: function( tween ) {
			var result;

			// Use a property on the element directly when it is not a DOM element,
			// or when there is no matching style property that exists.
			if ( tween.elem.nodeType !== 1 ||
				tween.elem[ tween.prop ] != null && tween.elem.style[ tween.prop ] == null ) {
				return tween.elem[ tween.prop ];

Review comment (suggestion):
The main difference is the addition of the `eased` parameter that seems redundant here. I'd check the size impact of that.


Code after:
Tween.prototype = {
	constructor: Tween,
	init: function( elem, options, prop, end, easing, unit ) {
		this.elem = elem;
		this.prop = prop;
		this.easing = easing || jQuery.easing._default;
		this.options = options;
		this.start = this.now = this.cur();
		this.end = end;
		this.unit = unit || ( jQuery.cssNumber[ prop ] ? "" : "px" );
	},
	cur: function() {
		var hooks = Tween.propHooks[ this.prop ];

		return hooks && hooks.get ?
			hooks.get( this ) :
			Tween.propHooks._default.get( this );
	},
	run: function( percent ) {
		var eased,
			hooks = Tween.propHooks[ this.prop ];

		if ( this.options.duration ) {
			this.pos = eased = jQuery.easing[ this.easing ](
				percent, this.options.duration * percent, 0, 1, this.options.duration
			);
		} else {
			this.pos = eased = percent;
		}
		this.now = ( this.end - this.start ) * eased + this.start;

		if ( this.options.step ) {
			this.options.step.call( this.elem, this.now, this );
		}

		if ( hooks && hooks.set ) {
			hooks.set( this );
		} else {
			Tween.propHooks._default.set( this );
		}
		return this;
	}
};

Tween.prototype.init.prototype = Tween.prototype;

Tween.propHooks = {
	_default: {
		get: function( tween ) {
			var result;
Repository: jquery/jquery
File: test/unit/core.js (JavaScript)

Code before:

	var i, elem, result;

	if ( typeof Symbol === "function" ) {

		elem = jQuery( "<div></div><span></span><a></a>" );
		result = "";

		try {
			eval( "for ( i of elem ) { result += i.nodeName; }" );
		} catch ( e ) {}
		assert.equal( result, "DIVSPANA", "for-of works on jQuery objects" );
	} else {
		assert.ok( true, "The browser doesn't support Symbols" );
	}
} );

QUnit[ jQuery.Deferred ? "test" : "skip" ]( "jQuery.readyException (original)", function( assert ) {
	assert.expect( 1 );

	var message,
		origWindowSetTimeout = window.setTimeout;

	window.setTimeout = function( fn ) {
		try {
			fn();
		} catch ( error ) {
			message = error.message;
		}
	};

	jQuery( function() {
		throw new Error( "Error in jQuery ready" );
	} );
	assert.strictEqual( message, "Error in jQuery ready", "The error wasn't thrown in a timeout" );
	window.setTimeout = origWindowSetTimeout;
} );

QUnit[ jQuery.Deferred ? "test" : "skip" ]( "jQuery.readyException (custom)", function( assert ) {
	assert.expect( 1 );

	var done = assert.async(),
		origReadyException = jQuery.readyException;

	jQuery.readyException = function( error ) {
		assert.strictEqual(
			error.message,
			"Error in jQuery ready",
			"The custom jQuery.readyException wasn't called"
		);
		done();

Review comment (suggestion):
For that case [ready-no-deferred](https://github.com/jquery/jquery/blob/25d8ccd1112d75394b91071ff7eba13283aaf898/src/core/ready-no-deferred.js) comes into play, right? Since it fires each handler in a timeout without a try/catch it's gonna dump a stack anyway.


Code after:
	function( assert, jQuery, window, document, error ) {
		assert.expect( 1 );
		assert.equal( error, false, "no call to user-defined onready" );
	}
);

QUnit.test( "Iterability of jQuery objects (gh-1693)", function( assert ) {
	assert.expect( 1 );

	var i, elem, result;

	if ( typeof Symbol === "function" ) {

		elem = jQuery( "<div></div><span></span><a></a>" );
		result = "";

		try {
			eval( "for ( i of elem ) { result += i.nodeName; }" );
		} catch ( e ) {}
		assert.equal( result, "DIVSPANA", "for-of works on jQuery objects" );
	} else {
		assert.ok( true, "The browser doesn't support Symbols" );
	}
} );

QUnit[ jQuery.Deferred ? "test" : "skip" ]( "jQuery.readyException (original)", function( assert ) {
	assert.expect( 1 );

	var message;

	this.sandbox.stub( window, "setTimeout", function( fn ) {
		try {
			fn();
		} catch ( error ) {
			message = error.message;
		}
	} );

	jQuery( function() {
		throw new Error( "Error in jQuery ready" );
	} );
	assert.strictEqual(
		message,
		"Error in jQuery ready",
		"The error should have been thrown in a timeout"
	);
} );

QUnit[ jQuery.Deferred ? "test" : "skip" ]( "jQuery.readyException (custom)", function( assert ) {
	assert.expect( 1 );
Repository: jquery/jquery
File: Gruntfile.js (JavaScript)

Code before:
						"ascii_only": true
					},
					banner: "/*! jQuery v<%= pkg.version %> | " +
						"(c) jQuery Foundation | jquery.org/license */",
					compress: {
						"hoist_funs": false,
						loops: false,
						unused: false
					}
				}
			}
		}
	} );

	// Load grunt tasks from NPM packages
	// Support: Node.js <4
	// Don't load the eslint task in old Node.js, it won't parse.
	require( "load-grunt-tasks" )( grunt, {
		pattern: oldNode ? [ "grunt-*", "!grunt-eslint" ] : [ "grunt-*" ]
	} );

	// Integrate jQuery specific tasks
	grunt.loadTasks( "build/tasks" );

	grunt.registerTask( "print_old_node_message", function() {
		var task = [].slice.call( arguments ).join( ":" );
		grunt.log.writeln( "Old Node.js detected, running the task \"" + task + "\" skipped..." );
	} );

	grunt.registerTask( "lint", [
		"jsonlint",
		runIfNewNode( "eslint:all" )
	] );

	// Don't run Node-related tests in Node.js < 1.0.0 as they require an old
	// jsdom version that needs compiling, making it harder for people to compile
	// jQuery on Windows. (see gh-2519)
	grunt.registerTask( "test_fast", [ runIfNewNode( "node_smoke_tests" ) ] );

	grunt.registerTask( "test", [ "test_fast" ].concat(
		[ runIfNewNode( "promises_aplus_tests" ) ]
	) );

	// Short list as a high frequency watch task
	grunt.registerTask( "dev", [
			"build:*:*",
			runIfNewNode( "newer:eslint:dev" ),
			"uglify",
			"remove_map_comment",
			"dist:*"
		]

Review comment (refactor):
> How would it help here?

I guess it would involve refactoring, it would be cool to aim to have minimum footprint for older node version, so we would remove in December in one move, anyway not insisting here


Code after:
					beautify: {
						"ascii_only": true
					},
					banner: "/*! jQuery v<%= pkg.version %> | " +
						"(c) jQuery Foundation | jquery.org/license */",
					compress: {
						"hoist_funs": false,
						loops: false,
						unused: false
					}
				}
			}
		}
	} );

	// Load grunt tasks from NPM packages
	// Support: Node.js <4
	// Don't load the eslint task in old Node.js, it won't parse.
	require( "load-grunt-tasks" )( grunt, {
		pattern: oldNode ? [ "grunt-*", "!grunt-eslint" ] : [ "grunt-*" ]
	} );

	// Integrate jQuery specific tasks
	grunt.loadTasks( "build/tasks" );

	grunt.registerTask( "print_old_node_message", function() {
		var task = [].slice.call( arguments ).join( ":" );
		grunt.log.writeln( "Old Node.js detected, running the task \"" + task + "\" skipped..." );
	} );

	grunt.registerTask( "lint", [
		"jsonlint",
		runIfNewNode( "eslint:all" )
	] );

	grunt.registerTask( "test_fast", [ runIfNewNode( "node_smoke_tests" ) ] );

	grunt.registerTask( "test", [ "test_fast" ].concat(
		[ runIfNewNode( "promises_aplus_tests" ) ]
	) );

	// Short list as a high frequency watch task
	grunt.registerTask( "dev", [
			"build:*:*",
			runIfNewNode( "newer:eslint:dev" ),
			"uglify",
			"remove_map_comment",
			"dist:*"
		]
	);
Repository: jquery/jquery
File: Gruntfile.js (JavaScript)

Code before:
						"ascii_only": true
					},
					banner: "/*! jQuery v<%= pkg.version %> | " +
						"(c) jQuery Foundation | jquery.org/license */",
					compress: {
						"hoist_funs": false,
						loops: false,
						unused: false
					}
				}
			}
		}
	} );

	// Load grunt tasks from NPM packages
	// Support: Node.js <4
	// Don't load the eslint task in old Node.js, it won't parse.
	require( "load-grunt-tasks" )( grunt, {
		pattern: oldNode ? [ "grunt-*", "!grunt-eslint" ] : [ "grunt-*" ]
	} );

	// Integrate jQuery specific tasks
	grunt.loadTasks( "build/tasks" );

	grunt.registerTask( "print_old_node_message", function() {
		var task = [].slice.call( arguments ).join( ":" );
		grunt.log.writeln( "Old Node.js detected, running the task \"" + task + "\" skipped..." );
	} );

	grunt.registerTask( "lint", [
		"jsonlint",
		runIfNewNode( "eslint:all" )
	] );

	// Don't run Node-related tests in Node.js < 1.0.0 as they require an old
	// jsdom version that needs compiling, making it harder for people to compile
	// jQuery on Windows. (see gh-2519)
	grunt.registerTask( "test_fast", [ runIfNewNode( "node_smoke_tests" ) ] );

	grunt.registerTask( "test", [ "test_fast" ].concat(
		[ runIfNewNode( "promises_aplus_tests" ) ]
	) );

	// Short list as a high frequency watch task
	grunt.registerTask( "dev", [
			"build:*:*",
			runIfNewNode( "newer:eslint:dev" ),
			"uglify",
			"remove_map_comment",
			"dist:*"
		]

Review comment (suggestion):
I just don't see how we could do it using `grunt.task.run` without that many modifications as in the current version of this PR.

Anyway, I tried to encapsulate it as much as possible, in most cases it will require just unwrapping the task name from `runIfNewNode()` so no biggie.


Code after:
					beautify: {
						"ascii_only": true
					},
					banner: "/*! jQuery v<%= pkg.version %> | " +
						"(c) jQuery Foundation | jquery.org/license */",
					compress: {
						"hoist_funs": false,
						loops: false,
						unused: false
					}
				}
			}
		}
	} );

	// Load grunt tasks from NPM packages
	// Support: Node.js <4
	// Don't load the eslint task in old Node.js, it won't parse.
	require( "load-grunt-tasks" )( grunt, {
		pattern: oldNode ? [ "grunt-*", "!grunt-eslint" ] : [ "grunt-*" ]
	} );

	// Integrate jQuery specific tasks
	grunt.loadTasks( "build/tasks" );

	grunt.registerTask( "print_old_node_message", function() {
		var task = [].slice.call( arguments ).join( ":" );
		grunt.log.writeln( "Old Node.js detected, running the task \"" + task + "\" skipped..." );
	} );

	grunt.registerTask( "lint", [
		"jsonlint",
		runIfNewNode( "eslint:all" )
	] );

	grunt.registerTask( "test_fast", [ runIfNewNode( "node_smoke_tests" ) ] );

	grunt.registerTask( "test", [ "test_fast" ].concat(
		[ runIfNewNode( "promises_aplus_tests" ) ]
	) );

	// Short list as a high frequency watch task
	grunt.registerTask( "dev", [
			"build:*:*",
			runIfNewNode( "newer:eslint:dev" ),
			"uglify",
			"remove_map_comment",
			"dist:*"
		]
	);
Repository: jquery/jquery
File: src/data.js (JavaScript)

Code before:
			return +data;
		default:
			if ( rbrace.test( data ) ) {
				return JSON.parse( data );
			}

			return data;
	}
}

function dataAttr( elem, key, data ) {
	var name;

	// If nothing was found internally, try to fetch any
	// data from the HTML5 data-* attribute
	if ( data === undefined && elem.nodeType === 1 ) {
		name = "data-" + key.replace( rmultiDash, "-$&" ).toLowerCase();
		data = elem.getAttribute( name );

		if ( typeof data === "string" ) {
			try {
				data = getData( data );
			} catch ( e ) {}

			// Make sure we set the data so it isn't changed later
			dataUser.set( elem, key, data );
		} else {
			data = undefined;
		}
	}
	return data;
}

jQuery.extend( {
	hasData: function( elem ) {
		return dataUser.hasData( elem ) || dataPriv.hasData( elem );
	},

	data: function( elem, name, data ) {
		return dataUser.access( elem, name, data );
	},

	removeData: function( elem, name ) {
		dataUser.remove( elem, name );
	},

	// TODO: Now that all calls to _data and _removeData have been replaced
	// with direct calls to dataPriv methods, these can be deprecated.
	_data: function( elem, name, data ) {
		return dataPriv.access( elem, name, data );
	},

Review comment (performance):
@mgol `switch..case` usually not optimized by the engines or the minificators :/, we can try `if..else` though  


Code after:
	}

	// Only convert to a number if it doesn't change the string
	if ( data === +data + "" ) {
		return +data;
	}

	if ( rbrace.test( data ) ) {
		return JSON.parse( data );
	}

	return data;
}

function dataAttr( elem, key, data ) {
	var name;

	// If nothing was found internally, try to fetch any
	// data from the HTML5 data-* attribute
	if ( data === undefined && elem.nodeType === 1 ) {
		name = "data-" + key.replace( rmultiDash, "-$&" ).toLowerCase();
		data = elem.getAttribute( name );

		if ( typeof data === "string" ) {
			try {
				data = getData( data );
			} catch ( e ) {}

			// Make sure we set the data so it isn't changed later
			dataUser.set( elem, key, data );
		} else {
			data = undefined;
		}
	}
	return data;
}

jQuery.extend( {
	hasData: function( elem ) {
		return dataUser.hasData( elem ) || dataPriv.hasData( elem );
	},

	data: function( elem, name, data ) {
		return dataUser.access( elem, name, data );
	},

	removeData: function( elem, name ) {
		dataUser.remove( elem, name );
	},

	// TODO: Now that all calls to _data and _removeData have been replaced
Repository: jquery/jquery
File: src/data.js (JavaScript)

Code before:
			return +data;
		default:
			if ( rbrace.test( data ) ) {
				return JSON.parse( data );
			}

			return data;
	}
}

function dataAttr( elem, key, data ) {
	var name;

	// If nothing was found internally, try to fetch any
	// data from the HTML5 data-* attribute
	if ( data === undefined && elem.nodeType === 1 ) {
		name = "data-" + key.replace( rmultiDash, "-$&" ).toLowerCase();
		data = elem.getAttribute( name );

		if ( typeof data === "string" ) {
			try {
				data = getData( data );
			} catch ( e ) {}

			// Make sure we set the data so it isn't changed later
			dataUser.set( elem, key, data );
		} else {
			data = undefined;
		}
	}
	return data;
}

jQuery.extend( {
	hasData: function( elem ) {
		return dataUser.hasData( elem ) || dataPriv.hasData( elem );
	},

	data: function( elem, name, data ) {
		return dataUser.access( elem, name, data );
	},

	removeData: function( elem, name ) {
		dataUser.remove( elem, name );
	},

	// TODO: Now that all calls to _data and _removeData have been replaced
	// with direct calls to dataPriv methods, these can be deprecated.
	_data: function( elem, name, data ) {
		return dataPriv.access( elem, name, data );
	},

Review comment (suggestion):
We should consider disabling `no-nested-ternary`... this is getting too aggressive for too little benefit (and even negative benefit, though I know which side of that argument I'm on).


Code after:
	}

	// Only convert to a number if it doesn't change the string
	if ( data === +data + "" ) {
		return +data;
	}

	if ( rbrace.test( data ) ) {
		return JSON.parse( data );
	}

	return data;
}

function dataAttr( elem, key, data ) {
	var name;

	// If nothing was found internally, try to fetch any
	// data from the HTML5 data-* attribute
	if ( data === undefined && elem.nodeType === 1 ) {
		name = "data-" + key.replace( rmultiDash, "-$&" ).toLowerCase();
		data = elem.getAttribute( name );

		if ( typeof data === "string" ) {
			try {
				data = getData( data );
			} catch ( e ) {}

			// Make sure we set the data so it isn't changed later
			dataUser.set( elem, key, data );
		} else {
			data = undefined;
		}
	}
	return data;
}

jQuery.extend( {
	hasData: function( elem ) {
		return dataUser.hasData( elem ) || dataPriv.hasData( elem );
	},

	data: function( elem, name, data ) {
		return dataUser.access( elem, name, data );
	},

	removeData: function( elem, name ) {
		dataUser.remove( elem, name );
	},

	// TODO: Now that all calls to _data and _removeData have been replaced
Repository: jquery/jquery
File: src/ajax/script.js (JavaScript)

Code before:
		"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;
	}
	if ( s.crossDomain ) {
		s.type = "GET";
	}
} );

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

	// This transport only deals with cross domain or forced-by-attrs requests
	if ( s.crossDomain || s.scriptAttrs ) {
		var script, callback;
		return {
			send: function( _, complete ) {
				script = jQuery( "<script>" ).prop( {
					charset: s.scriptCharset,
					src: s.url
				} ).attr( s.scriptAttrs || {} ).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
				document.head.appendChild( script[ 0 ] );
			},
			abort: function() {
				if ( callback ) {
					callback();
				}
			}
		};
	}
} );


Review comment (suggestion):
If `.prop(s.scriptAttrs)` is used like @jaubourg suggested maybe a single `.prop` call can be used, although probably depends on size.

Something such as:
```javascript
.prop( $.extend({}, s.scriptAttrs, {src: s.url, charset: s.scriptCharset} )
```

Code after:
		"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;
	}
	if ( s.crossDomain ) {
		s.type = "GET";
	}
} );

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

	// This transport only deals with cross domain or forced-by-attrs requests
	if ( 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
				document.head.appendChild( script[ 0 ] );
			},
			abort: function() {
				if ( callback ) {
					callback();
				}
			}
		};
	}
} );

} );
Repository: jquery/jquery
File: src/css/var/isHiddenWithinTree.js (JavaScript)

Code before:
define( [
	"../../core",
	"../../selector",
	"../../var/isAttached"

	// css is assumed
], function( jQuery, isAttached ) {
	"use strict";

	// isHiddenWithinTree reports if an element has a non-"none" display style (inline and/or
	// through the CSS cascade), which is useful in deciding whether or not to make it visible.
	// It differs from the :hidden selector (jQuery.expr.pseudos.hidden) in two important ways:
	// * A hidden ancestor does not force an element to be classified as hidden.
	// * Being disconnected from the document does not force an element to be classified as hidden.
	// These differences improve the behavior of .toggle() et al. when applied to elements that are
	// detached or contained within hidden ancestors (gh-2404, gh-2863).
	return function( elem, el ) {

		// isHiddenWithinTree might be called from jQuery#filter function;
		// in that case, element will be second argument
		elem = el || elem;

		// Inline style trumps all
		return elem.style.display === "none" ||
			elem.style.display === "" &&

			// Otherwise, check computed style
			// Support: Firefox <=43 - 45

Review comment (bug):
This is incorrect as this is the third parameter but it's used as a second one. You need to put that before `../../selector`. Could you run tests in AMD mode at least in one browser locally? It would catch those issues.

Unfortunately you can't do that via our Node.js setup but most non-AJAX tests should work without a PHP server. Or you can run one via `php -S localhost:7000` from your jQuery repository directory and open `http://localhost:7000`, choosing a module you modified (other than ajax) and enabling the AMD checkbox.

Code after:
define( [
	"../../core",
	"../../var/isAttached"

	// css is assumed
], function( jQuery, isAttached ) {
	"use strict";

	// isHiddenWithinTree reports if an element has a non-"none" display style (inline and/or
	// through the CSS cascade), which is useful in deciding whether or not to make it visible.
	// It differs from the :hidden selector (jQuery.expr.pseudos.hidden) in two important ways:
	// * A hidden ancestor does not force an element to be classified as hidden.
	// * Being disconnected from the document does not force an element to be classified as hidden.
	// These differences improve the behavior of .toggle() et al. when applied to elements that are
	// detached or contained within hidden ancestors (gh-2404, gh-2863).
	return function( elem, el ) {

		// isHiddenWithinTree might be called from jQuery#filter function;
		// in that case, element will be second argument
		elem = el || elem;

		// Inline style trumps all
		return elem.style.display === "none" ||
			elem.style.display === "" &&

			// Otherwise, check computed style
			// Support: Firefox <=43 - 45
			// Disconnected elements can have computed display: none, so first confirm that elem is
Repository: jquery/jquery
File: src/var/isAttached.js (JavaScript)

Code before:
define( [
	"../core",
	"../selector"
], function( jQuery ) {
	"use strict";

	return function isAttached( obj ) {
		return jQuery.contains( obj.ownerDocument, obj );
	};

} );


Review comment (suggestion):
Maybe add a comment that was previously in `curCSS`?
```
	"../selector" // Get jQuery.contains
```

Code after:
define( [
	"../core",
	"../selector" // Get jQuery.contains
], function( jQuery ) {
	"use strict";

	return function isAttached( obj ) {
		return jQuery.contains( obj.ownerDocument, obj );
	};

} );