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

Convert to eslint/prettier #2465

Merged
merged 5 commits into from Jun 15, 2023
Merged

Convert to eslint/prettier #2465

merged 5 commits into from Jun 15, 2023

Conversation

dancrumb
Copy link
Contributor

This change is limited to grpc-js for now for a few reasons:

  • That's really the scope of my immediate interest :)
  • Updating every package at once would make the PR much larger
  • This is the package that contains the most JS and TS

If you'd prefer to have every package updated, I'd suggest either:

  • I update this PR accordingly
  • I/we create an issue for each package and knock them out one at a time

Moving from exporting a namespace to just putting assert2 functions into their own files

Fixes grpc#2464
GTS provides config for ESLint, Prettier and TSConfig; this commit removes GTS, but brings over the configuration details

Fixes grpc#2464
…fig settings

This might actually be unnecessary; since I've copied over configuration settings from the GTS package, I figured I'd add this notice. It's in a file, since there's no capacity for adding comments in a JSON or .rc file. It feels doubtful that configuration settings fall under the auspices of the Apache License, but I'll leave that to the maintainers to decide.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 15, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@murgatroid99
Copy link
Member

I'm happy to just change grpc-js for now.

Did you need to make any manual changes to make the code conform to the lint rules?

@murgatroid99
Copy link
Member

FYI the Windows Test failure is not a concern here. It is a previously known failure unrelated to this change.

@@ -16,7 +16,7 @@
*/

import * as loader from '@grpc/proto-loader';
import * as assert from 'assert';
import * as assert2 from './assert2';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@murgatroid99 this is one material change I made (although it's really just a simple refactor).

It just removes the exporting of a namespace.

} catch (err) {
throw err;
}
const metadata: Metadata = await callCredentials.generateMetadata({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@murgatroid99 this is another material change; I removed the redundant try/catch (since the catch block just rethrows the caught error).

This was causing a lint error and seems redundant anyway.

@@ -147,12 +138,10 @@ describe('CallCredentials', () => {
await Promise.all(
testCases.map(async testCase => {
const { credentials, expected } = testCase;
let metadata: Metadata;
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@murgatroid99 this is another material change; I removed the redundant try/catch (since the catch block just rethrows the caught error).

This was causing a lint error and seems redundant anyway.

@@ -0,0 +1,4 @@
The following files contain configuration settings that were derived from [the this commit](https://github.com/google/gts/commit/3b9ab6dd59691f77f5c5c632a44c6762ba4ef7c6) in the Google GTS repository:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@murgatroid99 is this helpful, necessary, overkill? 🤷

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it's necessary, but I have no objection to including it.

Copy link
Member

@murgatroid99 murgatroid99 left a comment

Choose a reason for hiding this comment

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

Thank you for doing this work.

@murgatroid99 murgatroid99 merged commit dbaaa89 into grpc:master Jun 15, 2023
9 of 10 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants