Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Adding "allow-declarations" to only-arrow-functions #1452

Merged
merged 12 commits into from
Aug 9, 2016
Merged

Adding "allow-declarations" to only-arrow-functions #1452

merged 12 commits into from
Aug 9, 2016

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Aug 2, 2016

This allows standalone function declarations:

function foo() { }

...but still disallows function expressions inline:

var foo = function () { };
var bar = function () { };

Fixes #1449.

@JoshuaKGoldberg
Copy link
Contributor Author

I still get some small amount of amusement from tslint rules failing a PR to tslint. Will fix.

@@ -337,6 +337,7 @@ Core rules are included in the `tslint` package.
* `one-variable-per-declaration` disallows multiple variable definitions in the same statement.
* `"ignore-for-loop"` allows multiple variable definitions in for loop statement.
* `only-arrow-functions` disallows traditional `function () { ... }` declarations, preferring `() => { ... }` arrow lambdas.
* `"allow-declarations"` allows standalone function declarations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Changed how the README works, could you remove this change?

@jkillian
Copy link
Contributor

jkillian commented Aug 9, 2016

You'll need to remerge develop (sorry, I introduced conflicts!)

Also, could you rename test/rules/only-arrow-functions/full/ -> test/rules/only-arrow-functions/default/?

Also fixed up rule specifiers as per project guidelines.
@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Aug 9, 2016

ECONNRESET errors! 🙌

Otherwise it's all fixed.

Edit: I had neglected to push the test renames until 8/9. It should be fixed.

@@ -41,7 +55,9 @@ export class Rule extends Lint.Rules.AbstractRule {

class OnlyArrowFunctionsWalker extends Lint.RuleWalker {
public visitFunctionDeclaration(node: ts.FunctionDeclaration) {
this.addFailure(this.createFailure(node.getStart(), "function".length, Rule.FAILURE_STRING));
if (this.getOptions().indexOf(OPTION_ALLOW_DECLARATIONS) === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can just be this.hasOption(OPTION_ALLOW_DECLARATIONS)

@jkillian
Copy link
Contributor

jkillian commented Aug 9, 2016

I see we were making comments / changes at the same time! 😀

@JoshuaKGoldberg
Copy link
Contributor Author

Tests appear to be failing... I'll have to get to that later today.

@@ -55,7 +55,7 @@ export class Rule extends Lint.Rules.AbstractRule {

class OnlyArrowFunctionsWalker extends Lint.RuleWalker {
public visitFunctionDeclaration(node: ts.FunctionDeclaration) {
if (this.getOptions().indexOf(OPTION_ALLOW_DECLARATIONS) === -1) {
if (this.hasOption(OPTION_ALLOW_DECLARATIONS)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

my bad, I should have suggested !this.hasOption, I think the logic got inverted 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.

Can confirm.

@jkillian jkillian merged commit 6a81216 into palantir:master Aug 9, 2016
@jkillian
Copy link
Contributor

jkillian commented Aug 9, 2016

Merged!

@JoshuaKGoldberg JoshuaKGoldberg deleted the some-non-arrow-functions branch August 9, 2016 17:03
soniro pushed a commit to soniro/tslint that referenced this pull request Aug 31, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants