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

Add --show-diff cli option #165

Merged
merged 15 commits into from
Mar 13, 2023
Merged

Add --show-diff cli option #165

merged 15 commits into from
Mar 13, 2023

Conversation

skarab42
Copy link
Contributor

@skarab42 skarab42 commented Sep 20, 2022

This PR add the --show-diff cli option.

image

Some limitations:

  • The typeToString method of the TypeChecker does not parse all types in the same way. For example, an interface will return its name and not its contents, whereas an type alias will return its contents, which makes it difficult to compare the two types by string comparison.
  • The typeToString method of the TypeChecker does not allow the indentation of the inspected/resolved type to be preserved, so at the moment the types are displayed inline which is not very readable on complex types.

TODO:

  • Implement a normalized typeToString function.

@skarab42 skarab42 marked this pull request as draft September 20, 2022 09:30
source/cli.ts Outdated Show resolved Hide resolved
source/test/diff.ts Outdated Show resolved Hide resolved
@skarab42
Copy link
Contributor Author

skarab42 commented Sep 27, 2022

Currently the feature is only implemented in expectType and needs to be implemented in the other helpers that support it.
WIP... Done!

@skarab42 skarab42 changed the title Add --showDiff cli option Add --show-diff cli option Sep 27, 2022
@skarab42 skarab42 marked this pull request as ready for review September 27, 2022 13:46
@skarab42
Copy link
Contributor Author

TODO:

  • Implement a normalized typeToString function.

I can't work on this feature any time soon... Anyway I was thinking of doing it in another PR. So I won't add anything more in this PR. Waiting for your feedback for eventual changes.

source/cli.ts Outdated Show resolved Hide resolved
source/cli.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@tommy-mitchell tommy-mitchell left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to review. Besides some minor things, this looks good to me. How were you thinking of doing a normalized typeToString function? I could try implementing it.

@skarab42 skarab42 closed this Mar 11, 2023
@skarab42 skarab42 deleted the feat-diff branch March 11, 2023 07:17
@skarab42 skarab42 restored the feat-diff branch March 11, 2023 07:18
@skarab42 skarab42 reopened this Mar 11, 2023
@skarab42 skarab42 marked this pull request as draft March 11, 2023 07:25
@skarab42
Copy link
Contributor Author

How were you thinking of doing a normalized typeToString function? I could try implementing it.

You have to explore the AST of the type if it is not stringifiable, you have to browse its properties and format them. It's a huge job with a lot of edge cases to take into account.

@skarab42
Copy link
Contributor Author

@tommy-mitchell I don't know why the CI doesn't pass... I don't see any difference in the output and all the tests run locally under Windows and WLS. Sorry but I can't investigate further.

@skarab42 skarab42 marked this pull request as ready for review March 11, 2023 08:52
@tommy-mitchell tommy-mitchell mentioned this pull request Mar 11, 2023
@tommy-mitchell
Copy link
Contributor

Seems like the CI is failing because the diff cli test reports the location of the errors, but doesn't locally:

+'dist/test/fixtures/diff/index.test-d.ts:8:0',
 '✖   8:0  Parameter type { life?: number | undefined; } is declared too wide for argument type { life: number; }.',
 '',
 '- { life?: number | undefined; }',
 '+ { life: number; }',
 // ...

source/test/diff.ts Outdated Show resolved Hide resolved
@tommy-mitchell
Copy link
Contributor

Looks good to me.

@sindresorhus sindresorhus merged commit 8387d9e into tsdjs:main Mar 13, 2023
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.

3 participants