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

chore(typescript): turn on noImplicitOverride #7810

Merged
merged 1 commit into from Aug 25, 2021

Conversation

@JoelEinbinder
Copy link
Contributor

@JoelEinbinder JoelEinbinder commented Jul 23, 2021

TypeScript added a feature to force all overrides to be marked as such. We use to do this in the DevTools days with @override. We can land it if we like how it looks.

@dgozman
Copy link
Contributor

@dgozman dgozman commented Jul 27, 2021

What's the reason for this one? Also, why does it update all our deps?

@JoelEinbinder JoelEinbinder force-pushed the no_implicit_override branch from 8ff4211 to 52d2814 Aug 3, 2021
@JoelEinbinder
Copy link
Contributor Author

@JoelEinbinder JoelEinbinder commented Aug 3, 2021

What's the reason for this one? Also, why does it update all our deps?

Sorry I should've marked it as a draft because it depended on the the typescript roll. It should look a lot cleaner now. Added a description.

Copy link
Contributor

@dgozman dgozman left a comment

Seems ok, but also not too useful. Let's check what others think.

Copy link
Member

@mxschmitt mxschmitt left a comment

@JoelEinbinder JoelEinbinder force-pushed the no_implicit_override branch from 52d2814 to 85807cf Aug 24, 2021
@JoelEinbinder
Copy link
Contributor Author

@JoelEinbinder JoelEinbinder commented Aug 24, 2021

We have some code in utils.js where we override _write. The override keyword here makes it a lot more readable to me.

class HashStream extends stream.Writable {
  private _hash = crypto.createHash('sha1');

  override _write(chunk: Buffer, encoding: string, done: () => void) {
    this._hash.update(chunk);
    done();
  }

  digest(): string {
    return this._hash.digest('hex');
  }
}

We can always turn it off if people don't like it.

@JoelEinbinder JoelEinbinder merged commit fc991fe into microsoft:master Aug 25, 2021
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants