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

Remove explicit type anotations for inferrable types #293

Conversation

Roshanjossey
Copy link
Contributor

Refer conversation in #291 for information about this change

Refer conversation in #291 for information about this change
@pacman-bot
Copy link

pacman-bot commented Oct 16, 2019

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@SleeplessByte
Copy link
Member

@Roshanjossey can you do the others in THIS PR as well?

Re: your question:

https://github.com/exercism/typescript#shared-code

You can use https://github.com/exercism/typescript/blob/master/Makefile#L113-L123 to sync configurations to each exercise!

@Roshanjossey
Copy link
Contributor Author

Hi @SleeplessByte, just to be on the same page here, could you please confirm if my assumptions are correct?

  • Danger bot (pacman-bot) uses typescript-analyzer
  • The configurations in the exercise directories are used when users use exercism commandline and doing the exercises.

make test still throws errors in my local. So, please check at your end before merging this PR.

I'm still not comfortable with how yarn commands works in the individual exercises but we can talk about it in the prettier PR.

@SleeplessByte
Copy link
Member

Danger bot (pacman-bot) uses typescript-analyzer

No, it uses danger. The Dangerfile is executed on the eslint report (generated with make report) when we call danger, which is what we do on Travis.

The configurations in the exercise directories are used when users use exercism commandline and doing the exercises.

The exercism cli tool has nothing to do with any "local" configuration files. But exercism download will indeed download "the entire exercise directory" except for any .meta folder or any example.ts file.

I'm still not comfortable with how yarn commands works in the individual exercises but we can talk about it in the prettier PR.

I don't know what this means, but sure 😄

@SleeplessByte
Copy link
Member

make test still throws errors in my local. So, please check at your end before merging this PR.

@Roshanjossey Could you be so kind and attach that output? Or what are the errors you see?

@Roshanjossey
Copy link
Contributor Author

Roshanjossey commented Oct 18, 2019

This is what eslint . --ext .tsx,.ts returns when it's run as a part of make test

$ eslint . --ext .tsx,.ts

/tmp/tmp.dFNk9Uuwxk/beer-song.ts
  28:22  error  Type number trivially inferred from a number literal, remove type annotation  @typescript-eslint/no-inferrable-types
  28:40  error  Type number trivially inferred from a number literal, remove type annotation  @typescript-eslint/no-inferrable-types

/tmp/tmp.dFNk9Uuwxk/clock.ts
  5:29  error  Type number trivially inferred from a number literal, remove type annotation  @typescript-eslint/no-inferrable-types

/tmp/tmp.dFNk9Uuwxk/pangram.ts
  7:15  error  Type string trivially inferred from a string literal, remove type annotation  @typescript-eslint/no-inferrable-types

/tmp/tmp.dFNk9Uuwxk/pythagorean-triplet.ts
  35:34  error  Type number trivially inferred from a number literal, remove type annotation  @typescript-eslint/no-inferrable-types

/tmp/tmp.dFNk9Uuwxk/robot-simulator.ts
  5:15  error  Type number trivially inferred from a number literal, remove type annotation  @typescript-eslint/no-inferrable-types
  5:35  error  Type number trivially inferred from a number literal, remove type annotation  @typescript-eslint/no-inferrable-types
  5:55  error  Type string trivially inferred from a string literal, remove type annotation  @typescript-eslint/no-inferrable-types

✖ 8 problems (8 errors, 0 warnings)
  8 errors and 0 warnings potentially fixable with the `--fix` option.

error Command failed with exit code 1.

It's the same as what I was seeing earlier.
Danger bot reported the same things except changes for Atbash Cipher (those were inferred properties that was fixed in this PR) in #290.

@SleeplessByte
Copy link
Member

The tmp folder was probably not cleared correctly :)

@Roshanjossey
Copy link
Contributor Author

Tried with clearing everything from /tmp with the same results.

Could you try this branch on your local?

@SleeplessByte
Copy link
Member

Yep. Pulling it and trying it out.

@SleeplessByte SleeplessByte self-assigned this Oct 20, 2019
@SleeplessByte
Copy link
Member

Did a fresh clone and everything was ✔️ after I copied the shizzle to maintainers/.eslint.track.rc

@SleeplessByte SleeplessByte merged commit ff3f7d1 into exercism:master Oct 21, 2019
@Roshanjossey Roshanjossey deleted the remove-type-annotation-for-inferrable-properties branch October 22, 2019 07:32
@Roshanjossey
Copy link
Contributor Author

Awesome. Thank you @SleeplessByte ❤️

@SleeplessByte
Copy link
Member

Thank YOU for the work :D

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