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
Merged

alphabetize rule should cover exports as well #18

merged 12 commits into from
Oct 25, 2018

Conversation

alxndr
Copy link
Collaborator

@alxndr alxndr commented Oct 19, 2018

The current implementation of alphabetize-properties does not look at the names of specifiers in an export statement. Let's make it report those as well.

[FE-14]

...neovim didn't like the directory accoutrements
Copy link

@rohaus rohaus left a comment

Choose a reason for hiding this comment

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

Looks good!

}
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! :)

@alxndr alxndr changed the title FE-14 alphabetize rule should cover exports as well alphabetize rule should cover exports as well Oct 22, 2018
Copy link

@pcanterini pcanterini left a comment

Choose a reason for hiding this comment

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

Nice documentation!

.ignore Outdated
@@ -1,2 +1 @@
./node_modules/

node_modules/

Choose a reason for hiding this comment

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

can probably remove the trailing slash here


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...

@alxndr alxndr merged commit cba970a into master Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants