Skip to content
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

Jump to bottom

Upgrade Eslint from 3.18.0 to 4.19.0 #4820

Closed
wants to merge 12 commits into from
Closed

Upgrade Eslint from 3.18.0 to 4.19.0 #4820

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
28f20ed
Merge pull request #13 from oppia/develop
apb7 Feb 18, 2018
cdc2856
Merge pull request #14 from oppia/develop
apb7 Feb 22, 2018
af636ec
Merge pull request #15 from oppia/develop
apb7 Feb 25, 2018
860d44c
Merge pull request #16 from oppia/develop
apb7 Feb 26, 2018
0cdc197
Merge pull request #19 from oppia/develop
apb7 Mar 17, 2018
90e6e0c
Fix indent
apb7 Mar 18, 2018
01911b5
Change to alphabetical order
apb7 Mar 18, 2018
09c685c
Remove eslint-plugin-html
apb7 Mar 18, 2018
89fde6c
Revert: Add eslint-plugin-html
apb7 Mar 19, 2018
026e2eb
Add ajv and ajv-keywords dependency
apb7 Mar 19, 2018
8f3c058
Remove ajv and ajv-keywords statements from install_third_party.sh
apb7 Mar 19, 2018
e33f064
Review changes
apb7 Mar 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 13 additions & 10 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,17 @@
"eslint:recommended",
],
"plugins": [
"html",
"angular"
"angular",
"html"
],
"rules": {
"angular/di-unused": 2,
"angular/no-inline-template": [
"error",
{
"allowSimple": true
}
],
"brace-style":[
"error",
"1tbs"
Expand All @@ -24,13 +31,6 @@
"after": true
}
],
"angular/di-unused": 2,
"angular/no-inline-template": [
"error",
{
"allowSimple": true
}
],
"curly": [
"error",
"all"
Expand Down Expand Up @@ -83,6 +83,7 @@
]
}
],
"no-compare-neg-zero": "off",
"no-constant-condition": [
"off"
],
Expand Down Expand Up @@ -110,6 +111,7 @@
"no-unused-vars": [
"off"
],
"no-useless-escape": "off",
"one-var": [
"off"
],
Expand Down Expand Up @@ -143,7 +145,8 @@
"error",
{
"anonymous": "ignore",
"named": "never"
"named": "never",
"asyncArrow": "ignore"
}
],
"space-infix-ops": [
Expand Down
2 changes: 1 addition & 1 deletion core/templates/dev/head/components/CollectionCreationService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ oppia.factory('CollectionCreationService', [
}, function() {
$rootScope.loadingMessage = '';
}
);
);
Copy link
Member

Choose a reason for hiding this comment

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

This seems weird. Why is it indented at this level?

EDIT: Ah, it's because that's where the left paren opens. In this case move this to the end of the previous line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, got it!

}
};
}
Expand Down
8 changes: 4 additions & 4 deletions core/templates/dev/head/components/RatingDisplayDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ oppia.directive('ratingDisplay', [
for (var i = 0; i < $scope.stars.length; i++) {
$scope.stars[i].cssClass = (
ratingValue === undefined ? 'fa-star-o' :
ratingValue < $scope.stars[i].value - 0.75 ? 'fa-star-o' :
ratingValue < $scope.stars[i].value - 0.25 ? 'fa-star-half-o' :
'fa-star');
ratingValue < $scope.stars[i].value - 0.75 ? 'fa-star-o' :
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to force indent to happen only after "(", "[" and "{", but not be triggered by ":" or "?" or "&&", or ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did look into this. Eslint does not provide any such option to force indent.
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Sorry -- what I meant was more along the lines of, preventing additional indenting after ":", "?", etc. I'm not sure it makes sense for those things to start a "new scope", as brackets do.

Did this behaviour change recently with eslint? Is it worth writing a custom rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found out that there exists an option for ternary expressions, flatTernaryExpresions which is false by default. I have set it to true now. This would fix this problem.

ratingValue < $scope.stars[i].value - 0.25 ?
'fa-star-half-o' : 'fa-star');

if ($scope.status === STATUS_ACTIVE &&
ratingValue >= $scope.stars[i].value) {
Expand All @@ -86,7 +86,7 @@ oppia.directive('ratingDisplay', [
};
$scope.enterStar = function(starValue) {
if (
$scope.isEditable &&
$scope.isEditable &&
($scope.status === STATUS_ACTIVE ||
$scope.status === STATUS_INACTIVE)) {
$scope.status = STATUS_ACTIVE;
Expand Down
8 changes: 4 additions & 4 deletions core/templates/dev/head/components/StateGraphLayoutService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ oppia.factory('StateGraphLayoutService', [
if (numBacktrackingCalls <= MAX_BACKTRACKING_CALLS) {
for (var i = 0; i < adjacencyLists[currentNodeId].length; i++) {
if (currentPath.indexOf(
adjacencyLists[currentNodeId][i]) === -1) {
adjacencyLists[currentNodeId][i]) === -1) {
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have custom rule that indents this additionally by 2 (since the if clause spans a continuation line), similar to Python. OK to do this in a separate pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I have added it to my draft :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seanlip: I tried this out but the present indent rule raises an error if we indent L151 further by 2 spaces. Since consistency is still maintained throughout the codebase, we can adopt these indent settings. How does this sound?
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

If there isn't a way to enforce the "additional indent by 2" (similar to https://legacy.python.org/dev/peps/pep-0008/#indentation for python) then I'm fine with the new convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eslint offers indent-legacy rule but at the same time, their official documentation states:

The indent-legacy rule is deprecated and will not receive bugfixes or improvements in the future, so you should eventually switch back to the indent rule.
To address: We recommend upgrading without changing your indent configuration, and fixing any new indentation errors that appear in your codebase.

Therefore I think it would be better if we adopted the new convention.
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

OK, happy to defer to your call. Thanks for the explanation!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Let's go ahead with the new convention then. Thanks :)

backtrack(adjacencyLists[currentNodeId][i]);
}
}
Expand Down Expand Up @@ -244,7 +244,7 @@ oppia.factory('StateGraphLayoutService', [
nodeData[linkTarget].depth = nodeData[currNodeId].depth + 1;
nodeData[linkTarget].offset = (
nodeData[linkTarget].depth in maxOffsetInEachLevel ?
maxOffsetInEachLevel[nodeData[linkTarget].depth] + 1 : 0);
maxOffsetInEachLevel[nodeData[linkTarget].depth] + 1 : 0);

maxDepth = Math.max(maxDepth, nodeData[linkTarget].depth);
maxOffsetInEachLevel[nodeData[linkTarget].depth] = (
Expand Down Expand Up @@ -497,10 +497,10 @@ oppia.factory('StateGraphLayoutService', [
if (dx === 0 || dy !== 0) {
startCutoff = (
(dx === 0) ? (sourceHeight / 2) / Math.abs(dy) :
Math.min(startCutoff, (sourceHeight / 2) / Math.abs(dy)));
Math.min(startCutoff, (sourceHeight / 2) / Math.abs(dy)));
endCutoff = (
(dx === 0) ? (targetHeight / 2) / Math.abs(dy) :
Math.min(endCutoff, (targetHeight / 2) / Math.abs(dy)));
Math.min(endCutoff, (targetHeight / 2) / Math.abs(dy)));
}

var dxperp = targety - sourcey;
Expand Down
4 changes: 2 additions & 2 deletions core/templates/dev/head/components/create_button/CreateActivityButtonDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,11 @@ oppia.directive('createActivityButton', [

$scope.explorationImgUrl = (
UrlInterpolationService.getStaticImageUrl(
'/activity/exploration.svg'));
'/activity/exploration.svg'));

$scope.collectionImgUrl = (
UrlInterpolationService.getStaticImageUrl(
'/activity/collection.svg'));
'/activity/collection.svg'));
}],
windowClass: 'oppia-creation-modal'
}).result.then(function() {}, function() {
Expand Down
2 changes: 1 addition & 1 deletion core/templates/dev/head/components/forms/AudioTranslationsEditorDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ oppia.directive('audioTranslationsEditor', [
$scope.saveInProgress = false;
$scope.languageCode =
allowedAudioLanguageCodes.indexOf(prevLanguageCode) !== -1 ?
prevLanguageCode : allowedAudioLanguageCodes[0];
prevLanguageCode : allowedAudioLanguageCodes[0];
var uploadedFile = null;

$scope.isAudioTranslationValid = function() {
Expand Down
4 changes: 2 additions & 2 deletions core/templates/dev/head/components/forms/FormBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ oppia.filter('convertUnicodeWithParamsToHtml', ['$filter', function($filter) {
textFragments.forEach(function(fragment) {
result += (
fragment.type === 'text' ?
$filter('convertUnicodeToHtml')(fragment.data) :
'<oppia-parameter>' + fragment.data +
$filter('convertUnicodeToHtml')(fragment.data) :
'<oppia-parameter>' + fragment.data +
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand the indentation logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into the ternary operators. Thanks!

'</oppia-parameter>');
});
return result;
Expand Down
2 changes: 1 addition & 1 deletion core/templates/dev/head/components/forms/ImageUploaderDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ oppia.directive('imageUploader', [
var onDragEnd = function(e) {
e.preventDefault();
$('.image-uploader-drop-area').removeClass(
'image-uploader-is-active');
'image-uploader-is-active');
};

var validateUploadedFile = function(file, filename) {
Expand Down
2 changes: 1 addition & 1 deletion ...emplates/dev/head/components/forms/schema_editors/SchemaBasedExpressionEditorDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ oppia.directive('schemaBasedExpressionEditor', [
templateUrl: UrlInterpolationService.getDirectiveTemplateUrl(
'/components/forms/schema_editors/' +
'schema_based_expression_editor_directive.html'
),
),
restrict: 'E'
};
}]);
2 changes: 1 addition & 1 deletion core/templates/dev/head/components/forms/schema_editors/SchemaBasedListEditorDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ oppia.directive('schemaBasedListEditor', [
if ($scope.itemSchema().ui_config.coding_mode) {
$scope.isOneLineInput = false;
} else if (
$scope.itemSchema().ui_config.hasOwnProperty('rows') &&
$scope.itemSchema().ui_config.hasOwnProperty('rows') &&
$scope.itemSchema().ui_config.rows > 2) {
$scope.isOneLineInput = false;
}
Expand Down
14 changes: 7 additions & 7 deletions core/templates/dev/head/components/forms/schema_viewers/SchemaBasedUnicodeViewerDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ oppia.directive('schemaBasedUnicodeViewer', [
'schema_based_unicode_viewer_directive.html'),
restrict: 'E',
controller: [
'$scope', '$filter', '$sce',
function($scope, $filter, $sce) {
$scope.getDisplayedValue = function() {
return $sce.trustAsHtml($filter('convertUnicodeWithParamsToHtml')(
$scope.localValue));
};
}]
'$scope', '$filter', '$sce',
function($scope, $filter, $sce) {
$scope.getDisplayedValue = function() {
return $sce.trustAsHtml($filter('convertUnicodeWithParamsToHtml')(
$scope.localValue));
};
}]
};
}]);
2 changes: 1 addition & 1 deletion core/templates/dev/head/components/summary_tile/CollectionSummaryTileDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ oppia.directive('collectionSummaryTile', [
$scope.getCollectionLink = function() {
var targetUrl = (
$scope.isLinkedToEditorPage ?
COLLECTION_EDITOR_URL : COLLECTION_VIEWER_URL);
COLLECTION_EDITOR_URL : COLLECTION_VIEWER_URL);
return UrlInterpolationService.interpolateUrl(
targetUrl, {
collection_id: $scope.getCollectionId()
Expand Down
4 changes: 2 additions & 2 deletions core/templates/dev/head/components/summary_tile/ExplorationSummaryTileDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,9 @@ oppia.directive('explorationSummaryTile', [
contributorsSummary).sort(
function(contributorUsername1, contributorUsername2) {
var commitsOfContributor1 = contributorsSummary[
contributorUsername1].num_commits;
contributorUsername1].num_commits;
var commitsOfContributor2 = contributorsSummary[
contributorUsername2].num_commits;
contributorUsername2].num_commits;
return commitsOfContributor2 - commitsOfContributor1;
}
);
Expand Down
24 changes: 12 additions & 12 deletions core/templates/dev/head/domain/classifier/ClassifierObjectFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,17 @@
* domain objects.
*/

oppia.factory('ClassifierObjectFactory', [function() {
var Classifier = function(algorithmId, classifierData, dataSchemaVersion) {
this.algorithmId = algorithmId;
this.classifierData = classifierData;
this.dataSchemaVersion = dataSchemaVersion;
};
oppia.factory('ClassifierObjectFactory', [function() {
var Classifier = function(algorithmId, classifierData, dataSchemaVersion) {
this.algorithmId = algorithmId;
this.classifierData = classifierData;
this.dataSchemaVersion = dataSchemaVersion;
};

Classifier.create = function(
algorithmId, classifierData, dataSchemaVersion) {
return new Classifier(algorithmId, classifierData, dataSchemaVersion);
};
Classifier.create = function(
algorithmId, classifierData, dataSchemaVersion) {
return new Classifier(algorithmId, classifierData, dataSchemaVersion);
};

return Classifier;
}]);
return Classifier;
}]);
2 changes: 1 addition & 1 deletion core/templates/dev/head/domain/classifier/ClassifierObjectFactorySpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('Classifier Object Factory', function() {

it('should create a new classifier', function() {
var classifierObject = (
ClassifierObjectFactory.create(
ClassifierObjectFactory.create(
'TestClassifier', {}, 1));

expect(classifierObject.algorithmId).toEqual('TestClassifier');
Expand Down
4 changes: 2 additions & 2 deletions core/templates/dev/head/domain/collection/CollectionObjectFactorySpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,9 +423,9 @@ describe('Collection object factory', function() {
// Check get all skills.
var expectedSkills = {};
expectedSkills[skillId1] = CollectionSkillObjectFactory.createFromIdAndName(
skillId1, 'skill01');
skillId1, 'skill01');
expectedSkills[skillId2] = CollectionSkillObjectFactory.createFromIdAndName(
skillId2, 'skill02');
skillId2, 'skill02');
expect(_sampleCollection.getCollectionSkills()).toEqual(expectedSkills);
});

Expand Down
2 changes: 1 addition & 1 deletion core/templates/dev/head/domain/collection/EditableCollectionBackendApiServiceSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ describe('Editable collection backend API service', function() {
// Send a request to update collection
EditableCollectionBackendApiService.updateCollection(
collection.id, collection.version, collection.title, []).then(
successHandler, failHandler);
successHandler, failHandler);
$httpBackend.flush();

expect(successHandler).toHaveBeenCalledWith(collection);
Expand Down
2 changes: 1 addition & 1 deletion core/templates/dev/head/domain/collection/GuestCollectionProgressService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ oppia.factory('GuestCollectionProgressService', [
var recordCompletedExploration = function(collectionId, explorationId) {
var guestCollectionProgress = loadGuestCollectionProgress();
if (guestCollectionProgress.addCompletedExplorationId(
collectionId, explorationId)) {
collectionId, explorationId)) {
storeGuestCollectionProgress(guestCollectionProgress);
}
};
Expand Down
4 changes: 2 additions & 2 deletions core/templates/dev/head/domain/collection/SearchExplorationsBackendApiServiceSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ describe('Exploration search backend API service', function() {

$httpBackend.expect(
'GET', '/exploration/metadata_search?q=' + query).respond(200, {
collection_node_metadata_list: []
});
collection_node_metadata_list: []
});
SearchExplorationsBackendApiService.fetchExplorations('three').then(
successHandler, failHandler);
$httpBackend.flush();
Expand Down
8 changes: 4 additions & 4 deletions core/templates/dev/head/domain/exploration/EditableExplorationBackendApiServiceSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ describe('Editable exploration backend API service', function() {
EditableExplorationBackendApiService.updateExploration(
exploration.exploration_id, exploration.version,
exploration.title, []).then(
successHandler, failHandler);
successHandler, failHandler);
$httpBackend.flush();

expect(successHandler).toHaveBeenCalledWith(exploration);
Expand Down Expand Up @@ -187,7 +187,7 @@ describe('Editable exploration backend API service', function() {
EditableExplorationBackendApiService.updateExploration(
exploration.exploration_id, exploration.version,
exploration.title, []).then(
successHandler, failHandler);
successHandler, failHandler);
$httpBackend.flush();

expect(successHandler).toHaveBeenCalledWith(exploration);
Expand Down Expand Up @@ -221,7 +221,7 @@ describe('Editable exploration backend API service', function() {
EditableExplorationBackendApiService.updateExploration(
exploration.exploration_id, exploration.version,
'Minor edits', []).then(
successHandler, failHandler);
successHandler, failHandler);
$httpBackend.flush();

expect(successHandler).toHaveBeenCalledWith(exploration);
Expand All @@ -230,7 +230,7 @@ describe('Editable exploration backend API service', function() {
$httpBackend.expect('DELETE', '/createhandler/data/0').respond({});
EditableExplorationBackendApiService.deleteExploration(
exploration.exploration_id).then(
successHandler, failHandler);
successHandler, failHandler);
$httpBackend.flush();

expect(successHandler).toHaveBeenCalledWith({});
Expand Down