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.

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

alphabetize rule should cover exports as well #18

Merged
merged 12 commits into from
Oct 25, 2018
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
node_modules
3 changes: 1 addition & 2 deletions .ignore
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
./node_modules/

node_modules
10 changes: 8 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,22 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog] and this project adheres to [Semantic Versioning].


## unreleased (2018-10-23)

### Changed
- `alphabetize-properties` rule applies to `export` statement.


## [1.3.0] (2018-04-27)

### Added
- Many (non-custom) rules from ESLint, `eslint-plugin-babel`, and `eslint-plugin-react`.
- Many (non-custom) rules from ESLint, `eslint-plugin-babel`, and `eslint-plugin-react` to the Recommended Ruleset.


## [1.2.0] (2018-03-23)

### Added
- Recommended ruleset
- Recommended Ruleset


## [1.1.1] (2017-06-01)
Expand Down
25 changes: 23 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,26 @@ Then configure any rules you'd like to tweak under the `rules` section:
## Contributions

We welcome contributions!
There are a few ideas in the Issues of this repo.
There are a few ideas in the [Issues] of this repo.

To get started with how to lint JavaScript, play around with the [AST Explorer].

We use a form of [git-flow]; please create any Pull Requests based on the `develop` branch.
Please add tests.

Please add tests!


## Release Process

This codebase attempts to follow [Semantic Versioning].

1. One or more *approved* Pull Requests are merged to the `master` branch.

Choose a reason for hiding this comment

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

More of a FYI but I'm a big fan of always using 1. for all items in an MD ordered list to keep it simple since it will always render the correct order.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm yeah... I like having it be readable in plain-text mode as well, although I suppose having all 1s isn't super unreadable...

1. Ensure the test suite passes, and runs the expected number of tests: `npm test`
1. Choose the appropriate New Version Number according to [Semantic Versioning]. (The `CHANGELOG.md` file may help with identifying the nature of the changes.)
1. Edit the `CHANGELOG.md` file, replacing the "Unreleased" label with the New Version Number, linked to the (as-yet-uncreated) GitHub tag as well (the pattern is `https://github.com/<user>/<repo>/releases/tag/<version-number>`). Ensure that the notes under this version reflect the changes made by the merged Pull Requests.
1. Run `npm version <version-number> --message "Version %s"` to both update the version in the `package.json` file and create the tag on GitHub.
1. Run `git push && npm publish` to push the merges & version to GitHub, and publish the new package on npmjs.com.




Expand All @@ -86,7 +103,11 @@ Please add tests.
[no-assign-in-case-without-braces]: ./lib/rules/no-assign-in-case-without-braces.md
[no-unauthorized-global-properties]: ./lib/rules/no-unauthorized-global-properties.md
[no-use-entire-process-dot-env]: ./lib/rules/no-use-entire-process-dot-env.md
[npm version]: https://docs.npmjs.com/getting-started/publishing-npm-packages#how-to-update-a-package
[prefer-includes-over-indexof]: ./lib/rules/prefer-includes-over-indexof.md
[AST Explorer]: https://astexplorer.net/
[ESLint]: http://eslint.org
[Issues]: https://github.com/bleacherreport/eslint-plugin-laws-of-the-game/issues
[PillarsOfJS]: https://medium.com/javascript-scene/the-two-pillars-of-javascript-ee6f3281e7f3
[Process-dot-env]: https://nodejs.org/api/process.html#process_process_env
[Semantic Versioning]: https://semver.org/spec/v2.0.0.html
32 changes: 25 additions & 7 deletions lib/rules/alphabetize-properties.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,16 @@ function extractKey(objectProperty) {
return objectProperty.key.name;
}

function extractName(exportSpecifier) {
return exportSpecifier.local.name;
}

function checkOrderOfKeysIn(array) {
return function isKeyOutOfOrder(key, index) {
return index !== 0 && key < array[index-1];
};
}

module.exports = {
esLintRuleName: "alphabetize-properties-of-large-objects",
schema: [
Expand Down Expand Up @@ -49,26 +59,34 @@ module.exports = {
throw new Error("Value of `exempt` option must be an array of strings (key names); saw a: " + typeof exemptions);
}
var removeExemptions = makeExemptionsFilter(exemptions);
function checkExport(node) {
if (!node.specifiers.length) {
return;
}
var rawExportNames = node.specifiers.map(extractName);
if (rawExportNames.length < limit) {
return;
}
var exportNames = rawExportNames.filter(removeExemptions);
if (exportNames.find(checkOrderOfKeysIn(exportNames))) {
context.report({message: "Exports with "+limit+" or more specifiers should have specifiers in alphabetical order. (Saw "+rawExportNames.length+")", node: node});
Copy link

Choose a reason for hiding this comment

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

Could we create the message using template strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had been keeping this as plain-JS as possible, with the hopes that it'll be able to work on as older versions of JS/ES... But that might not be as not as important as the readability that template strings and arrow functions offer?

Copy link

Choose a reason for hiding this comment

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

Ohh good point, then this is fine! :)

}
}
function checkObject(node) {
if (!node.properties.length) {
// somewhat-earlier exit...
return;
}
var rawPropertyKeys = node.properties.filter(isStandardProperty).map(extractKey);
if (rawPropertyKeys.length < limit) {
return;
}
var propertyKeys = rawPropertyKeys.filter(removeExemptions);
function isKeyOutOfOrder(key, index) {
// TODO control sorting order?
return index !== 0 && key < propertyKeys[index-1];
}

if (propertyKeys.find(isKeyOutOfOrder)) {
if (propertyKeys.find(checkOrderOfKeysIn(propertyKeys))) {
context.report({message: "Objects with "+limit+" or more properties should have properties in alphabetical order. (Saw "+rawPropertyKeys.length+")", node: node});
}
}
return {
"ExportNamedDeclaration": checkExport,
"ObjectExpression": checkObject
};
}
Expand Down
6 changes: 6 additions & 0 deletions lib/rules/alphabetize-properties.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ The specific limit of "how many properties is too many" is configurable. It defa

Individual properties can also be named as being exempt from the alphabetization rule. This is to address a situation where a specific key should always occur first or last in an object, while keeping the remaining properties alphabetized.

This rule applies to object literals as well as named exports.


## Rule Details

Expand All @@ -14,11 +16,13 @@ The following patterns are considered warnings:
```js
/* eslint alphabetize-properties */
obj = {z:z, a:a, b:b, c:c, d:d}
export {z:z, a:a, b:b, c:c, d:d}
```

```js
/* eslint alphabetize-properties: [2, {limit: 2}] */
obj = {z:z, a:a}
export {z:z, a:a}
```

The following patterns are not considered warnings:
Expand All @@ -28,11 +32,13 @@ The following patterns are not considered warnings:
obj = {}
obj = {z:z, a:a}
obj = {a:a, b:b, c:c, d:d, z:z}
export {a:a, b:b, c:c, d:d, z:z}
```

```js
/* eslint alphabetize-properties: [2, {except: ["mixins"]}] */
obj = {}
obj = {z:z, a:a}
obj = {a:a, b:b, c:c, d:d, z:z, mixins: [foo]}
export {a:a, b:b, c:c, d:d, z:z, mixins: [foo]}
```
8 changes: 8 additions & 0 deletions tests/lib/rules/alphabetize-properties.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ var rule = require("../../../lib/rules/alphabetize-properties");
testHelper.tester.run("alphabetize-properties", rule, {

valid: [
"export {}",
"export {z, a}",
"export {a, b, c, d, z}",
{code: "export {a, z, y, b}", options: [{limit: 3, exempt: ["y", "z"]}]},
"obj = {}",
"obj = {z:z, a:a}",
"obj = {a:a, b:b, c:c, d:d, z:z}",
Expand All @@ -15,6 +19,10 @@ testHelper.tester.run("alphabetize-properties", rule, {
],

invalid: [
"export {z, a, b, c, d}",
["export {z, a}", [{"limit": 2}]],
["export {a, z, b}", [{limit: 3, exempt: ["a"]}]],
["export {a, z, y, b}", [{limit: 3, exempt: ["a", "z"]}]],
"obj = {z:z, a:a, b:b, c:c, d:d}",
"obj = {...foo, z:z, a:a, b:b, c:c, d:d}",
["obj = {z:z, a:a}", [{"limit": 2}]],
Expand Down