Organize imports #2125

mauriciovieira · 2017-02-15T02:51:26Z

remove unused imports
organize imports (sort and unify comments)
remove unused eslint-disable

mauriciovieira · 2017-02-15T03:21:49Z

@apinf/developers please review

mauriciovieira · 2017-02-15T03:23:57Z

Notice I standardized the first comments with:

// Meteor packages imports

// Npm packages imports

// Collection imports

// APINF imports

The imports follow this order unless they break eslint

brylie · 2017-02-15T08:34:19Z

I am wondering whether removing all of these explicit imports is the 'right thing to do'. In other words, it may be useful, and at some point necessary, to explicitly import Meteor components. Consistency with imports also helps our developers to better understand how things are connected, and is a common practice with other frameworks that don't have such a 'magical' environment as Meteor.

brylie · 2017-02-15T09:35:51Z

The Meteor Guide also recommends using explicit imports for Meteor libraries:

it is recommended best practice that you first load all the Meteor “pseudo-globals” using the import { Name } from 'meteor/package' syntax before using them.

Meteor Guide: Application Structure - Importing Meteor “pseudo-globals”

mauriciovieira · 2017-02-15T10:24:59Z

What I mind mostly is the consistency. Before we started to run eslint (a month ago) we had meteor imports only in some files.

brylie · 2017-02-15T10:33:30Z

Yes, for the same reason we had lint in most files. In effect, the strict import has not been a problem for the duration of our development, and has only recently been caught by the linter. Hence, the work in progress towards consistent use of explicit imports -- while this PR effectively reverses our work in progress.

mauriciovieira · 2017-02-15T10:35:32Z

No problem, I'll write some script to put them back (removing using sed was easy).
I spent a few hours yesterday sorting and commenting all files and won't like to waste this work.

brylie · 2017-02-15T10:37:13Z

Cool. I am not trying to give you grief. I'm just clarifying a direction we are heading, based on the recommendation of the Meteor Guide (as well as the eslint-airbnb style from which we inherit most of our lint rules).

mauriciovieira · 2017-02-19T11:29:57Z

Use node to reinsert the import statements http://nicolashery.com/parse-data-files-using-nodejs-streams/ (https://github.com/nicolashery/example-stream-parser/blob/master/lib/parser.js)

brylie · 2017-02-20T10:33:59Z

Is this ready for review? It might be easier to just create a new branch, since most of the changes were unnecessary. Also, I am not sure we should add 'meteor' to the eslint environment, since it might discourage developers from being explicit about Meteor imports.

mauriciovieira · 2017-02-28T05:21:25Z

I prefer not undoing the manual work I've done to comment and sort all imports.

I wanted to work on a script to automatically fix the meteor no-undef. This script might be useful. I am still working on that.

1. remove unused imports 2. organize imports (sort and unify comments) 3. remove unused eslint-disable closes #2107

- Identify which packages are missing - Make eslint to ignore scripts/ directory

mauriciovieira · 2017-03-02T02:57:25Z

@brylie I rebased the commits.

I standardized the first comments with:

// Meteor packages imports

// Meteor contributed packages imports

// Npm packages imports

// Collection imports

// APINF imports

Also, I noticed some broken references in the develop branch and fixed them.

Please review.

mauriciovieira · 2017-03-02T03:03:48Z

'Add meteor env to eslintrc' is not a suitable description for this work. It's better to call it 'Organize imports'. I renamed it

marla-singer · 2017-03-02T06:53:38Z

api_docs/client/swaggerUI/swaggerUiContent/swaggerUiContent.js

 import { Meteor } from 'meteor/meteor';
 import { Template } from 'meteor/templating';

-import Apis from '/apis/collection';
+// Meteor contributed packages imports
 import ApiKeys from '/api_keys/collection';


Hm. Looks like it's Collection imports not Meteor contributed packages imports

marla-singer · 2017-03-02T06:56:21Z

api_keys/collection/schema.js

 import { SimpleSchema } from 'meteor/aldeed:simple-schema';

+// Npm packages imports
 import ApiKeys from './';


Collection imports here

marla-singer · 2017-03-02T06:58:41Z

apis/client/profile/intro/intro.js

-// Allow use of 'document' as global object
-/* global document */
+// Meteor packages imports
+import { Template } from 'meteor/templating';

 // Meteor imports
 import { TAPi18n } from 'meteor/tap:i18n';


Meteor contributed packages imports here

marla-singer · 2017-03-02T07:02:22Z

apis/server/methods/status.js

 import { Meteor } from 'meteor/meteor';
+import { check } from 'meteor/check';
+

 import Apis from '/apis/collection';


Collection imports here

marla-singer · 2017-03-02T07:05:21Z

users/collection/users_permissions.js

 import { Meteor } from 'meteor/meteor';

+


Excessive new line

marla-singer · 2017-03-02T07:32:51Z

core/client/error_templates/go_back_button/goBackButton.js

 import { Template } from 'meteor/templating';

+


Excessive new line

marla-singer · 2017-03-02T07:33:28Z

dashboard/client/charts/table/table.js

 import { ReactiveVar } from 'meteor/reactive-var';
+import { Template } from 'meteor/templating';
+



Excessive new line

marla-singer · 2017-03-02T07:34:33Z

feedback/client/list/list.js

 import { Template } from 'meteor/templating';

+import { Modal } from 'meteor/peppelg:bootstrap-3-modal';
+
 import Feedback from '../../collection';


Collection imports here

marla-singer · 2017-03-02T07:34:54Z

feedback/collection/schema.js

 import { Meteor } from 'meteor/meteor';
+
 import { SimpleSchema } from 'meteor/aldeed:simple-schema';
 import Feedback from './';


Collection imports here

marla-singer · 2017-03-02T07:35:16Z

home/client/footer/homeFooter.js

 import { Template } from 'meteor/templating';

+


Excessive new line

marla-singer · 2017-03-02T07:40:38Z

proxy_backends/client/modal_form/remove_selected_proxy/remove_selected_proxy.js

@@ -0,0 +1,28 @@
+// Meteor packages imports
+import { Template } from 'meteor/templating';


@mauriciovieira removed_selected_proxy folder was deleted

Oh, thanks for catching this :-)

marla-singer · 2017-03-02T07:42:35Z

status/client/status.js

 import { Meteor } from 'meteor/meteor';
 import { Template } from 'meteor/templating';

+


Excessive new line

marla-singer · 2017-03-02T07:42:45Z

status/server/status.js

 import { Meteor } from 'meteor/meteor';

+


Excessive new line

marla-singer · 2017-03-02T07:43:18Z

users/client/profile/profile.js

 import { Meteor } from 'meteor/meteor';
 import { Template } from 'meteor/templating';

+


Excessive new line

mauriciovieira · 2017-03-02T12:07:49Z

@marla-singer Thank you so much for catching these errors.
Please review again.

mauriciovieira added the in progress label Feb 15, 2017

mauriciovieira force-pushed the feature/eslint-meteor-env branch from 1eac760 to 9d8ba9e Compare February 15, 2017 03:21

mauriciovieira added the Ready for review label Feb 15, 2017

brylie self-assigned this Feb 15, 2017

brylie removed the Ready for review label Feb 15, 2017

mauriciovieira force-pushed the feature/eslint-meteor-env branch from 92426f8 to 2a46bc1 Compare February 26, 2017 09:22

brylie modified the milestone: Sprint 37 Feb 27, 2017

mauriciovieira added 2 commits March 1, 2017 22:56

Add meteor env to eslintrc

a6e5701

1. remove unused imports 2. organize imports (sort and unify comments) 3. remove unused eslint-disable closes #2107

Remove meteor env

b5c80e8

mauriciovieira force-pushed the feature/eslint-meteor-env branch from 3763173 to 7852a7c Compare March 2, 2017 02:00

mauriciovieira added 3 commits March 1, 2017 23:02

Parse eslint output

2213034

- Identify which packages are missing - Make eslint to ignore scripts/ directory

Parse errors and pass to a fixer

e427176

Reinsert meteor packages imports

2ea9ce6

mauriciovieira force-pushed the feature/eslint-meteor-env branch from 7852a7c to 2ea9ce6 Compare March 2, 2017 02:05

Merge develop into feature/eslint-meteor-env

415ce57

mauriciovieira added the Ready for review label Mar 2, 2017

mauriciovieira changed the title ~~Add meteor env to eslintrc~~ Organize imports Mar 2, 2017

marla-singer reviewed Mar 2, 2017

View reviewed changes

brylie assigned marla-singer and unassigned brylie Mar 2, 2017

mauriciovieira added 2 commits March 2, 2017 09:06

Remove previously removed file

8d93470

Organize imports

17faa52

marla-singer approved these changes Mar 2, 2017

View reviewed changes

marla-singer merged commit 0e3430b into develop Mar 2, 2017

marla-singer removed in progress Ready for review labels Mar 2, 2017

marla-singer deleted the feature/eslint-meteor-env branch March 2, 2017 13:28

		import { ReactiveVar } from 'meteor/reactive-var';
		import { Template } from 'meteor/templating';

		@@ -0,0 +1,28 @@
		// Meteor packages imports
		import { Template } from 'meteor/templating';

		import { Meteor } from 'meteor/meteor';
		import { Template } from 'meteor/templating';

Organize imports #2125

Organize imports #2125

Conversation

mauriciovieira commented Feb 15, 2017

mauriciovieira commented Feb 15, 2017

mauriciovieira commented Feb 15, 2017 • edited

brylie commented Feb 15, 2017

brylie commented Feb 15, 2017 • edited

mauriciovieira commented Feb 15, 2017

brylie commented Feb 15, 2017 • edited

mauriciovieira commented Feb 15, 2017

brylie commented Feb 15, 2017 • edited

mauriciovieira commented Feb 19, 2017

brylie commented Feb 20, 2017

mauriciovieira commented Feb 28, 2017

mauriciovieira commented Mar 2, 2017

mauriciovieira commented Mar 2, 2017

marla-singer Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marla-singer Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovieira Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovieira commented Mar 2, 2017

mauriciovieira commented Feb 15, 2017 •

edited

brylie commented Feb 15, 2017 •

edited

brylie commented Feb 15, 2017 •

edited

brylie commented Feb 15, 2017 •

edited

marla-singer Mar 2, 2017 •

edited

marla-singer Mar 2, 2017 •

edited

mauriciovieira Mar 2, 2017 •

edited