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

An expression of type 'void' cannot be tested for truthiness #9012

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

roni-frantchi
Copy link

@roni-frantchi roni-frantchi commented Dec 24, 2018

Closes #8968 .
See PR by @navneetkarnani for more details and trial.

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/typescript-angular-all-petstore.sh and ./bin/security/{LANG}-petstore.sh.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

The Typescript 3.1 compiler (one that is used with Angular 7 onwards), has a check for void functions being checked for Truthiness.
This change drops the or (||) operator altogether, extracts the assignment code to runtime and utilizes the comma (,) operator instead.

@tengis
Copy link

tengis commented Jan 15, 2019

Hi Rony,

What's outstanding for this PR?

Cheers

@roni-frantchi
Copy link
Author

Hi @tengis .
This PR was made as a suggestion or an alternative to #8968 (comment) .
Haven't heard back from those who have made the original PR, nor the core maintainers... I'm waiting for a code review, really.

In the meantime I'm seeing yet another fix here #9065 by @marcbuils .
Maybe between the three of us we can bring one of these PRs forward?.. I'd love a review by anyone.

@@ -262,7 +262,8 @@ export class {{classname}} {
{{#hasFormParams}}
const canConsumeForm = this.canConsumeForm(consumes);

let formParams: { append(param: string, value: any): void; };
let formParams: FormParams;
Copy link

@emmanuelgautier emmanuelgautier Feb 22, 2019

Choose a reason for hiding this comment

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

formParams could be a HttpParams type and need to not use the return as the same way.

Copy link
Author

Choose a reason for hiding this comment

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

@emmanuelgautier FormParams type is defined here as a union type that includes HttpParams, so I think that would work. What am I missing?
What do you mean by "and need not use the return as the same way"?

Choose a reason for hiding this comment

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

FormData and HttpParams append function does not have the same signature :

One return FormParams and the other not. When you use formParams.append, you have to check what is the exact variable type. Here, you do not make this check and could be a potential error when formParams is a HttpParams and you use HttpClient

It is almost the same thing that I done in this lines.

Copy link
Author

Choose a reason for hiding this comment

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

@emmanuelgautier: They differ on the return type, that is the reason why in my method, I implicitly check the type (by checking the useReturnValue ) -
If it's HttpParams I return the return value of append(), if it isn't, I append, and return the same instance (see the comma , there).

Choose a reason for hiding this comment

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

Here, if you use HttpClient, you do not use useReturnValue but formParams could be a HttpParams and return value.

Copy link
Author

Choose a reason for hiding this comment

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

How could formParams be HttpParams? the only line that assigns an HttpParams instance to formParam is this one, which is then always followed by a new assignment to the appender here

Choose a reason for hiding this comment

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

You are right. My bad 👍

DsAekb added a commit to apaleo/swagger-codegen that referenced this pull request Apr 9, 2019
@sashaaro
Copy link

sashaaro commented Jul 3, 2019

is there any news? Probably it was fixed in here OpenAPITools/openapi-generator@0a45890#diff-f61af8a4c4eb2720bfb15259e0dca419

@HugoMario HugoMario mentioned this pull request Jul 16, 2020
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.

4 participants