Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

Fix: Optional methods are not marked as optional (fixes #206) #207

Merged
merged 1 commit into from
Apr 1, 2017

Conversation

Pajn
Copy link
Contributor

@Pajn Pajn commented Mar 20, 2017

No description provided.

@eslintbot
Copy link

LGTM

@soda0289
Copy link
Member

I don't think you can have optional parameters defined in a class. This would only be valid when the class is declared

declare class A {
  optional?();
  required();
}

or maybe even an abstract class

abstract class A {
  public abstract optional?();
  
  public abstract required();
}

The typescript parser should throw an error not sure why its not.

@soda0289
Copy link
Member

soda0289 commented Mar 20, 2017

I could be wrong thou it seems to work on typescript playground just seems odd to me.
I should note it works when I remove the empty body required() function:

class A {
  public  optional?();
}

@Pajn
Copy link
Contributor Author

Pajn commented Mar 20, 2017

If I understand the tests correctly optional properties are not allowed but optional methods are
https://github.com/Microsoft/TypeScript/blob/ac67b94a6ea209760bfc31d9328337a16c41a0c1/tests/cases/conformance/types/namedTypes/classWithOptionalParameter.ts
https://github.com/Microsoft/TypeScript/blob/ac67b94a6ea209760bfc31d9328337a16c41a0c1/tests/cases/conformance/types/namedTypes/optionalMethods.ts

However, non of the show errors in my editor so it seems I have stumbled upon a strange corner case 😛

@soda0289
Copy link
Member

I never knew of that typescript feature. I think we may want to add tests for the other two cases, declare and abstract, and maybe not have a class with a required function without a body since that should be an error in typescript.

@soda0289
Copy link
Member

I see the test case does not have a required function without a body. Never mind this should be good.

@Pajn
Copy link
Contributor Author

Pajn commented Mar 20, 2017

ah, sorry for that. I focused on the optional part and didn't realize the example in the issue was invalid. But as you noticed, that isn't the same as the test :)

I will add one ambient/declare and one abstract class as test cases as well.

@eslintbot
Copy link

LGTM

@soda0289 soda0289 merged commit 1e73711 into eslint:master Apr 1, 2017
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.

4 participants