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

promise-function-async: add options to specify function types to check. #3807

Conversation

blair
Copy link
Contributor

@blair blair commented Apr 4, 2018

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

If one wants to enable the promise-function-async warning, then one gets warnings for all functions, including arrow functions that are passed to Promise#then() and Promise#catch(). There's no point in making these async then then() and catch() already handle the given function throwing an exception.

This commit allows one to select which function types to check. A useful option is to only check function and method declarations.

Question: after this PR, all the warnings will be disabled by default. Should they all be enabled if there are no options set?

CHANGELOG.md entry:

[rule-change] promise-function-async add rules options to enable checks for the four different types of functions.

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @blair! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@blair blair force-pushed the promise-function-async-add-optional-function-type-checking branch from e094ec7 to cf9028d Compare April 4, 2018 06:02
Copy link
Contributor

@suchanlee suchanlee left a comment

Choose a reason for hiding this comment

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

LGTM! Just the one comment regarding default options

minLength: 0,
maxLength: 4,
},
optionExamples: [[true, OPTION_FUNCTION_DECLARATION, OPTION_METHOD_DECLARATION]],
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can add two examples here, the one that you added and the other true?


type EnabledSyntaxKinds = ReadonlySet<number>;

function parseOptions(ruleArguments: string[]): EnabledSyntaxKinds {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think if this rule is enabled we should default to all kinds. Or else what's the point of turning the rule on in the first place?

This is useful if you only want your function and method declarations
to warn, or in other words, one doesn't want each arrow function that
is passed to Promise#then() or Promise#catch() to be async.
@blair blair force-pushed the promise-function-async-add-optional-function-type-checking branch from cf9028d to 459ee72 Compare April 7, 2018 02:27
@blair
Copy link
Contributor Author

blair commented Apr 7, 2018

@suchanlee Please take another look, I made the requested changes.

BTW, I didn't know if I should have a new commit on top of the original one or replace it, I did the later with a forced push.

@suchanlee
Copy link
Contributor

Thanks for the contribution @blair! Replacing the commit is fine.

@suchanlee suchanlee merged commit 375de6d into palantir:master Apr 7, 2018
HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018
…k. (palantir#3807)

This is useful if you only want your function and method declarations
to warn, or in other words, one doesn't want each arrow function that
is passed to Promise#then() or Promise#catch() to be async.
@blair blair deleted the promise-function-async-add-optional-function-type-checking branch April 29, 2018 17:23
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.

3 participants