Skip to content

Update code style, partially use new qunit interface, give test module a new name #2542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
markelog wants to merge 6 commits into from
Closed

Update code style, partially use new qunit interface, give test module a new name #2542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cd04a4c
Tests: Partially use new qunit interface
markelog Aug 16, 2015
b83dfa7
Tests: Do not define two modules with the same name
markelog Aug 16, 2015
b5caff3
Build: Update jscs and lint files
markelog Aug 16, 2015
447e2a8
Build: correct jscs paths
markelog Sep 2, 2015
38f5822
Build: correct style tests files which could be automatically corrected
markelog Sep 2, 2015
f4c2202
Ajax: do not quote "throws" option - use dot notation instead
markelog Sep 3, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .jscsrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
{
"preset": "jquery",

// remove after https://github.com/jscs-dev/node-jscs/issues/1685
// and https://github.com/jscs-dev/node-jscs/issues/1686
"requireCapitalizedComments": null,

"excludeFiles": [ "external", "src/intro.js", "src/outro.js",
"test/node_smoke_tests/lib/ensure_iterability.js" ]
"test/node_smoke_tests/lib/ensure_iterability.js", "node_modules" ]
}
19 changes: 12 additions & 7 deletions Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ module.exports = function( grunt ) {
// But our modules can
delete srcHintOptions.onevar;

grunt.initConfig({
grunt.initConfig( {
pkg: grunt.file.readJSON( "package.json" ),
dst: readOptionalJSON( "dist/.destination.json" ),
"compare_size": {
Expand Down Expand Up @@ -53,6 +53,7 @@ module.exports = function( grunt ) {
"core",
"selector"
],

// Exclude specified modules if the module matching the key is removed
removeWith: {
ajax: [ "manipulation/_evalUrl", "event/ajax" ],
Expand Down Expand Up @@ -104,13 +105,17 @@ module.exports = function( grunt ) {
}
},
jscs: {
src: "src/**/*.js",
src: "src",
gruntfile: "Gruntfile.js",

// Check parts of tests that pass
test: [ "test/data/testrunner.js", "test/unit/animation.js", "test/unit/tween.js" ],
release: [ "build/*.js", "!build/release-notes.js" ],
tasks: "build/tasks/*.js"
test: [
"test/data/testrunner.js",
"test/unit/animation.js",
"test/unit/tween.js",
"test/unit/wrap.js"
],
build: "build"
},
testswarm: {
tests: [
Expand Down Expand Up @@ -162,7 +167,7 @@ module.exports = function( grunt ) {
}
}
}
});
} );

// Load grunt tasks from NPM packages
require( "load-grunt-tasks" )( grunt );
Expand All @@ -177,7 +182,7 @@ module.exports = function( grunt ) {
grunt.registerTask( "test", [ "test_fast", "promises_aplus_tests" ] );

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

grunt.registerTask( "default", [ "dev", "test_fast", "compare_size" ] );
};
7 changes: 4 additions & 3 deletions build/release.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module.exports = function( Release ) {

npmTags = Release.npmTags;

Release.define({
Release.define( {
npmPublish: true,
issueTracker: "github",
/**
Expand All @@ -36,6 +36,7 @@ module.exports = function( Release ) {
* for publishing the distribution repo instead
*/
npmTags: function() {

// origRepo is not defined if dist was skipped
Release.dir.repo = Release.dir.origRepo || Release.dir.repo;
return npmTags();
Expand All @@ -47,9 +48,9 @@ module.exports = function( Release ) {
dist: function( callback ) {
cdn.makeArchives( Release, function() {
dist( Release, callback );
});
} );
}
});
} );
};

module.exports.dependencies = [
Expand Down
51 changes: 36 additions & 15 deletions build/tasks/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,16 @@ module.exports = function( grunt ) {
baseUrl: "src",
name: "jquery",
out: "dist/jquery.js",

// We have multiple minify steps
optimize: "none",

// Include dependencies loaded with require
findNestedDependencies: true,

// Avoid inserting define() placeholder
skipModuleInsertion: true,

// Avoid breaking semicolons inserted by r.js
skipSemiColonInsertion: true,
wrap: {
Expand All @@ -47,22 +51,25 @@ module.exports = function( grunt ) {
*/
function convert( name, path, contents ) {
var amdName;

// Convert var modules
if ( /.\/var\//.test( path ) ) {
contents = contents
.replace( /define\([\w\W]*?return/, "var " + (/var\/([\w-]+)/.exec(name)[1]) + " =" )
.replace( /define\([\w\W]*?return/, "var " + ( /var\/([\w-]+)/.exec( name )[ 1 ] ) + " =" )
.replace( rdefineEnd, "" );

// Sizzle treatment
} else if ( /^sizzle$/.test( name ) ) {
contents = "var Sizzle =\n" + contents

// Remove EXPOSE lines from Sizzle
.replace( /\/\/\s*EXPOSE[\w\W]*\/\/\s*EXPOSE/, "return Sizzle;" );

} else {

contents = contents
.replace( /\s*return\s+[^\}]+(\}\s*?\);[^\w\}]*)$/, "$1" )

// Multiple exports
.replace( /\s*exports\.\w+\s*=\s*\w+;/g, "" );

Expand All @@ -82,13 +89,15 @@ module.exports = function( grunt ) {
contents = contents
.replace( /define\(\[[^\]]*\]\)[\W\n]+$/, "" );
}

// AMD Name
if ( (amdName = grunt.option( "amd" )) != null && /^exports\/amd$/.test( name ) ) {
if (amdName) {
if ( ( amdName = grunt.option( "amd" ) ) != null && /^exports\/amd$/.test( name ) ) {
Copy link
Member

@gibson042 gibson042 Aug 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can changes like this wait for *SpacesInsideParenthesizedExpression?

Copy link
Member Author

@markelog markelog Aug 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule itself is not such a big thing, when (and again, thousands apologies i still didn't get around to that request) this rule will be landed, we would need to update our code style, meaning, we would need to argue and probably vote again, after that, we would need to update documentation on the site, after that, we would need to add it to the jquery preset and after that, we would need to wait on next the minor release of jscs.

That might take some time, whereas we have our code-style in current form, on which we all agreed upon. Whereas, if we would decide on that, we could auto-format it in the same easy way i did in this pull.

Copy link
Member

@timmywil timmywil Aug 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whereas, if we would decide on that, we could auto-format it in the same easy way i did in this pull.

That's a good point. I don't think we need to wait for that in order to land this PR.

Copy link
Member

@gibson042 gibson042 Aug 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't advocating blocking the PR, just backing out the changes pertaining to spaces in parenthesized expressions (which I believe would shrink it considerably, but I suppose would require a temporary requireSpacesInsideParentheses override).

As for revoting/updating documentation/etc., a rule requiring such spaces doesn't appear in http://contribute.jquery.org/style-guide/js/#spacing or the old .jscs.json, and in fact was stealth-added (from the perspective of this codebase, not its contributors) by jscs-dev/node-jscs@0f78baf in response to jquery/contribute.jquery.org#104 (which was about requiring spaces in function calls). Since you committed it, I can just directly ask how you would have treated expression parentheses if JSCS were capable of differentiating them. But I will point out that I haven't encountered one argument in favor of requiring them that didn't hinge upon style guide automation, which falls apart as soon as that process becomes sufficiently sophisticated.

Copy link
Member Author

@markelog markelog Aug 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which was about requiring spaces in function calls

Well, in that discussion Mike suggested to remove all exceptions, so we wouldn't create sophisticated logic for the parentheses rules in JSCS needed only for the one preset, that was somewhat the point of that proposal.

I can just directly ask how you would have treated expression parentheses if JSCS were capable of differentiating them

I would follow the style guide, no matter of it is, i.e. jquery preset should represent it.

Personally, i would add more exceptions to that rule as i suggested in jquery/contribute.jquery.org#104 including the "expression parentheses"

Copy link
Member

@timmywil timmywil Aug 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm less bothered by the extra spaces in expression parenthesis as I am by the extra space at the end of our methods (i.e. } );), which I don't think would require changes to jscs if we were comfortable with things like method( arg1, { object: value }).

Copy link
Member

@mgol mgol Aug 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I will point out that I haven't encountered one argument in favor of requiring them that didn't hinge upon style guide automation

I have one - it's just simpler to remember that an opening paren containing something is always followed by a space. Simpler for us, simpler for contributors.

But as long as the tooling enforces the agreed convention, I'm fine whatever we end up with.

Copy link
Member

@mgol mgol Aug 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm less bothered by the extra spaces in expression parenthesis as I am by
the extra space at the end of our methods (i.e. } );), which I don't
think would require changes to jscs if we were comfortable with things like method(
arg1, { object: value }).

I like those spaces for consistency reasons. If the majority doesn't want
them, I'm fine with this exception (although I prefer not to have too many
exceptions as @mikesherov suggested) but it'd be good to have it checked or
we'll have half of the code in one style & half in the other.

Copy link
Member

@timmywil timmywil Aug 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I'm okay with leaving it as-is. If we agree to change it, we'll do another PR.

if ( amdName ) {
grunt.log.writeln( "Naming jQuery with AMD name: " + amdName );
} else {
grunt.log.writeln( "AMD name now anonymous" );
}

// Remove the comma for anonymous defines
contents = contents
.replace( /(\s*)"jquery"(\,\s*)/, amdName ? "$1\"" + amdName + "\"$2" : "" );
Expand Down Expand Up @@ -121,7 +130,8 @@ module.exports = function( grunt ) {
excludeList = function( list, prepend ) {
if ( list ) {
prepend = prepend ? prepend + "/" : "";
list.forEach(function( module ) {
list.forEach( function( module ) {

// Exclude var modules as well
if ( module === "var" ) {
excludeList(
Expand All @@ -130,20 +140,22 @@ module.exports = function( grunt ) {
return;
}
if ( prepend ) {

// Skip if this is not a js file and we're walking files in a dir
if ( !(module = /([\w-\/]+)\.js$/.exec( module )) ) {
if ( !( module = /([\w-\/]+)\.js$/.exec( module ) ) ) {
return;
}

// Prepend folder name if passed
// Remove .js extension
module = prepend + module[1];
module = prepend + module[ 1 ];
}

// Avoid infinite recursion
if ( excluded.indexOf( module ) === -1 ) {
excluder( "-" + module );
}
});
} );
}
},
/**
Expand All @@ -158,12 +170,15 @@ module.exports = function( grunt ) {
module = m[ 2 ];

if ( exclude ) {

// Can't exclude certain modules
if ( minimum.indexOf( module ) === -1 ) {

// Add to excluded
if ( excluded.indexOf( module ) === -1 ) {
grunt.log.writeln( flag );
excluded.push( module );

// Exclude all files in the folder of the same name
// These are the removable dependencies
// It's fine if the directory is not there
Expand All @@ -173,10 +188,11 @@ module.exports = function( grunt ) {
grunt.verbose.writeln( e );
}
}

// Check removeWith list
excludeList( removeWith[ module ] );
} else {
grunt.log.error( "Module \"" + module + "\" is a minimum requirement.");
grunt.log.error( "Module \"" + module + "\" is a minimum requirement." );
if ( module === "selector" ) {
grunt.log.error(
"If you meant to replace Sizzle, use -sizzle instead."
Expand Down Expand Up @@ -215,13 +231,13 @@ module.exports = function( grunt ) {

// Handle Sizzle exclusion
// Replace with selector-native
if ( (index = excluded.indexOf( "sizzle" )) > -1 ) {
if ( ( index = excluded.indexOf( "sizzle" ) ) > -1 ) {
config.rawText.selector = "define(['./selector-native']);";
excluded.splice( index, 1 );
}

// Replace exports/global with a noop noConflict
if ( (index = excluded.indexOf( "exports/global" )) > -1 ) {
if ( ( index = excluded.indexOf( "exports/global" ) ) > -1 ) {
config.rawText[ "exports/global" ] = "define(['../core']," +
"function( jQuery ) {\njQuery.noConflict = function() {};\n});";
excluded.splice( index, 1 );
Expand All @@ -233,9 +249,11 @@ module.exports = function( grunt ) {
// append excluded modules to version
if ( excluded.length ) {
version += " -" + excluded.join( ",-" );

// set pkg.version to version with excludes, so minified file picks it up
grunt.config.set( "pkg.version", version );
grunt.verbose.writeln( "Version changed to " + version );

// Have to use shallow or core will get excluded since it is a dependency
config.excludeShallow = excluded;
}
Expand All @@ -247,8 +265,10 @@ module.exports = function( grunt ) {
*/
config.out = function( compiled ) {
compiled = compiled

// Embed Version
.replace( /@VERSION/g, version )

// Embed Date
// yyyy-mm-ddThh:mmZ
.replace( /@DATE/g, ( new Date() ).toISOString().replace( /:\d+\.\d+Z$/, "Z" ) );
Expand All @@ -259,9 +279,10 @@ module.exports = function( grunt ) {

// Turn off opt-in if necessary
if ( !optIn ) {

// Overwrite the default inclusions with the explicit ones provided
config.rawText.jquery = "define([" +
(included.length ? included.join(",") : "") +
( included.length ? included.join( "," ) : "" ) +
"]);";
}

Expand All @@ -272,8 +293,8 @@ module.exports = function( grunt ) {
done();
}, function( err ) {
done( err );
});
});
} );
} );

// Special "alias" task to make custom build creation less grawlix-y
// Translation example
Expand All @@ -289,6 +310,6 @@ module.exports = function( grunt ) {

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

grunt.task.run([ "build:*:*" + (modules ? ":" + modules : ""), "uglify", "dist" ]);
});
grunt.task.run( [ "build:*:*" + ( modules ? ":" + modules : "" ), "uglify", "dist" ] );
} );
};
14 changes: 7 additions & 7 deletions build/tasks/dist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ module.exports = function( grunt ) {
flags = Object.keys( this.flags );

// Combine all output target paths
paths = [].concat( stored, flags ).filter(function( path ) {
paths = [].concat( stored, flags ).filter( function( path ) {
return path !== "*";
});
} );

// Ensure the dist files are pure ASCII
nonascii = false;

distpaths.forEach(function( filename ) {
distpaths.forEach( function( filename ) {
var i, c,
text = fs.readFileSync( filename, "utf8" );

Expand All @@ -53,7 +53,7 @@ module.exports = function( grunt ) {
}

// Optionally copy dist files to other locations
paths.forEach(function( path ) {
paths.forEach( function( path ) {
var created;

if ( !/\/$/.test( path ) ) {
Expand All @@ -63,9 +63,9 @@ module.exports = function( grunt ) {
created = path + filename.replace( "dist/", "" );
grunt.file.write( created, text );
grunt.log.writeln( "File '" + created + "' created." );
});
});
} );
} );

return !nonascii;
});
} );
};
2 changes: 1 addition & 1 deletion build/tasks/install_jsdom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ module.exports = function( grunt ) {
args: [ "install", "jsdom@" + version ],
opts: { stdio: "inherit" }
}, this.async() );
});
} );
};
3 changes: 2 additions & 1 deletion build/tasks/sourcemap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ var fs = require( "fs" );
module.exports = function( grunt ) {
var minLoc = Object.keys( grunt.config( "uglify.all.files" ) )[ 0 ];
grunt.registerTask( "remove_map_comment", function() {

// Remove the source map comment; it causes way too many problems.
// The map file is still generated for manual associations
// https://github.com/jquery/jquery/issues/1707
var text = fs.readFileSync( minLoc, "utf8" )
.replace( /\/\/# sourceMappingURL=\S+/, "" );
fs.writeFileSync( minLoc, text );
});
} );
};
Loading