Skip to content

build: Re-enable eslint no-unused-vars, no-control-regex and no-loss-of-precision #10049

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 4, 2024
Merged

build: Re-enable eslint no-unused-vars, no-control-regex and no-loss-of-precision #10049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
59f8647
build: Re-enable eslint `no-unused-vars`, `no-control-regex` and `no-…
lforst Jan 4, 2024
8ea94aa
Merge branch 'develop' into lforst-re-enable-biome-disabled-rules
lforst Jan 4, 2024
55313f9
Unfuck metrics interface
lforst Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,11 @@ module.exports = {
'types/**',
],
overrides: [
{
files: ['*'],
rules: {
// Disabled because it's included with Biome's linter
'no-control-regex': 'off',
},
},
{
files: ['*.ts', '*.tsx', '*.d.ts'],
parserOptions: {
project: ['tsconfig.json'],
},
rules: {
// Disabled because it's included with Biome's linter
'@typescript-eslint/no-unused-vars': 'off',
'@typescript-eslint/no-loss-of-precision': 'off',
},
},
{
files: ['test/**/*.ts', 'test/**/*.tsx'],
Expand Down
2 changes: 1 addition & 1 deletion dev-packages/browser-integration-tests/suites/public-api/startSpan/error-sync/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ sentryTest('should capture an error within a sync startSpan callback', async ({
const gotoPromise = page.goto(url);
const envelopePromise = getMultipleSentryEnvelopeRequests<Event>(page, 2);

const [_, events] = await Promise.all([gotoPromise, envelopePromise]);
const [, events] = await Promise.all([gotoPromise, envelopePromise]);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you changed the order of Promise.all array, you wouldn't have [, syntax

const txn = events.find(event => event.type === 'transaction');
const err = events.find(event => !event.type);

Expand Down
2 changes: 1 addition & 1 deletion dev-packages/node-integration-tests/suites/anr/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ conditionalTest({ min: 16 })('should report ANR when event loop blocked', () =>
done();
}, 5_000);

childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {
childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, () => {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
hasClosed = true;
});
});
Expand Down
4 changes: 2 additions & 2 deletions packages/core/test/lib/exports.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@ describe('withScope', () => {
});

it('works with a return value', () => {
const res = withScope(scope => {
const res = withScope(() => {
return 'foo';
});

expect(res).toBe('foo');
});

it('works with an async function return value', async () => {
const res = withScope(async scope => {
const res = withScope(async () => {
return 'foo';
});

Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/lib/utils/applyScopeDataToEvent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,9 @@ describe('mergeScopeData', () => {
const breadcrumb2 = { message: '2' } as Breadcrumb;
const breadcrumb3 = { message: '3' } as Breadcrumb;

const eventProcessor1 = ((a: unknown) => null) as EventProcessor;
const eventProcessor2 = ((b: unknown) => null) as EventProcessor;
const eventProcessor3 = ((c: unknown) => null) as EventProcessor;
const eventProcessor1 = (() => null) as EventProcessor;
const eventProcessor2 = (() => null) as EventProcessor;
const eventProcessor3 = (() => null) as EventProcessor;

const data1: ScopeData = {
eventProcessors: [eventProcessor1],
Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/cron/node-cron.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export function instrumentNodeCron<T>(lib: Partial<NodeCron> & T): T {
// When 'get' is called for schedule, return a proxied version of the schedule function
return new Proxy(target.schedule, {
apply(target, thisArg, argArray: Parameters<NodeCron['schedule']>) {
const [expression, _, options] = argArray;
const [expression, , options] = argArray;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not in favor of sparse arrays. There is a specific eslint rule (and also for Biome) to avoid this: https://eslint.org/docs/latest/rules/no-sparse-arrays


if (!options?.name) {
throw new Error('Missing "name" for scheduled job. A name is required for Sentry check-in monitoring.');
Expand Down
4 changes: 2 additions & 2 deletions packages/node/test/cron.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('cron check-ins', () => {

const CronJobWithCheckIn = cron.instrumentCron(CronJobMock, 'my-cron-job');

const _ = new CronJobWithCheckIn('* * * Jan,Sep Sun', () => {
new CronJobWithCheckIn('* * * Jan,Sep Sun', () => {
expect(withMonitorSpy).toHaveBeenCalledTimes(1);
expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), {
schedule: { type: 'crontab', value: '* * * 1,9 0' },
Expand All @@ -67,7 +67,7 @@ describe('cron check-ins', () => {

const CronJobWithCheckIn = cron.instrumentCron(CronJobMock, 'my-cron-job');

const _ = CronJobWithCheckIn.from({
CronJobWithCheckIn.from({
cronTime: '* * * Jan,Sep Sun',
onTick: () => {
expect(withMonitorSpy).toHaveBeenCalledTimes(1);
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry/src/custom/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export function wrapClientClass<
event: Event,
hint: EventHint,
scope?: Scope,
isolationScope?: Scope,
_isolationScope?: Scope,
): PromiseLike<Event | null> {
let actualScope = scope;

Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry/src/spanProcessor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { maybeCaptureExceptionForTimedEvent } from './utils/captureExceptionForT
import { getHubFromContext } from './utils/contextData';
import { getSpanHub, setSpanFinishScope, setSpanHub, setSpanParent, setSpanScope } from './utils/spanData';

function onSpanStart(span: Span, parentContext: Context, ScopeClass: typeof OpenTelemetryScope): void {
function onSpanStart(span: Span, parentContext: Context, _ScopeClass: typeof OpenTelemetryScope): void {
// This is a reliable way to get the parent span - because this is exactly how the parent is identified in the OTEL SDK
const parentSpan = trace.getSpan(parentContext);
const hub = getHubFromContext(parentContext);
Expand Down
18 changes: 8 additions & 10 deletions packages/serverless/test/awslambda.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ const fakeCallback: Callback = (err, result) => {
return err;
};

function expectScopeSettings(fakeTransactionContext: any) {
// @ts-expect-error see "Why @ts-expect-error" note
const fakeSpan = { ...SentryNode.fakeSpan, ...fakeTransactionContext };
function expectScopeSettings() {
// @ts-expect-error see "Why @ts-expect-error" note
expect(SentryNode.fakeScope.setTransactionName).toBeCalledWith('functionName');
// @ts-expect-error see "Why @ts-expect-error" note
Expand Down Expand Up @@ -213,7 +211,7 @@ describe('AWSLambda', () => {

expect(rv).toStrictEqual(42);
expect(SentryNode.startSpanManual).toBeCalledWith(fakeTransactionContext, expect.any(Function));
expectScopeSettings(fakeTransactionContext);
expectScopeSettings();
// @ts-expect-error see "Why @ts-expect-error" note
expect(SentryNode.fakeSpan.end).toBeCalled();
expect(SentryNode.flush).toBeCalledWith(2000);
Expand All @@ -239,7 +237,7 @@ describe('AWSLambda', () => {
};

expect(SentryNode.startSpanManual).toBeCalledWith(fakeTransactionContext, expect.any(Function));
expectScopeSettings(fakeTransactionContext);
expectScopeSettings();
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-expect-error see "Why @ts-expect-error" note
expect(SentryNode.fakeSpan.end).toBeCalled();
Expand Down Expand Up @@ -317,7 +315,7 @@ describe('AWSLambda', () => {
};

expect(SentryNode.startSpanManual).toBeCalledWith(fakeTransactionContext, expect.any(Function));
expectScopeSettings(fakeTransactionContext);
expectScopeSettings();
expect(SentryNode.captureException).toBeCalledWith(e, expect.any(Function));
// @ts-expect-error see "Why @ts-expect-error" note
expect(SentryNode.fakeSpan.end).toBeCalled();
Expand Down Expand Up @@ -345,7 +343,7 @@ describe('AWSLambda', () => {

expect(rv).toStrictEqual(42);
expect(SentryNode.startSpanManual).toBeCalledWith(fakeTransactionContext, expect.any(Function));
expectScopeSettings(fakeTransactionContext);
expectScopeSettings();
// @ts-expect-error see "Why @ts-expect-error" note
expect(SentryNode.fakeSpan.end).toBeCalled();
expect(SentryNode.flush).toBeCalled();
Expand Down Expand Up @@ -382,7 +380,7 @@ describe('AWSLambda', () => {
};

expect(SentryNode.startSpanManual).toBeCalledWith(fakeTransactionContext, expect.any(Function));
expectScopeSettings(fakeTransactionContext);
expectScopeSettings();
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-expect-error see "Why @ts-expect-error" note
expect(SentryNode.fakeSpan.end).toBeCalled();
Expand Down Expand Up @@ -425,7 +423,7 @@ describe('AWSLambda', () => {

expect(rv).toStrictEqual(42);
expect(SentryNode.startSpanManual).toBeCalledWith(fakeTransactionContext, expect.any(Function));
expectScopeSettings(fakeTransactionContext);
expectScopeSettings();
// @ts-expect-error see "Why @ts-expect-error" note
expect(SentryNode.fakeSpan.end).toBeCalled();
expect(SentryNode.flush).toBeCalled();
Expand Down Expand Up @@ -462,7 +460,7 @@ describe('AWSLambda', () => {
};

expect(SentryNode.startSpanManual).toBeCalledWith(fakeTransactionContext, expect.any(Function));
expectScopeSettings(fakeTransactionContext);
expectScopeSettings();
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-expect-error see "Why @ts-expect-error" note
expect(SentryNode.fakeSpan.end).toBeCalled();
Expand Down
14 changes: 4 additions & 10 deletions packages/types/src/metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,21 @@ import type { Primitive } from './misc';
* An abstract definition of the minimum required API
* for a metric instance.
*/
export abstract class MetricInstance {
export interface MetricInstance {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we make this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it was incorrect. We do not want this to be an abstract class. Nothing actually extended it. I made this change specifically in this PR because previously the value parameter in the add method was unused, causing linting errors.

/**
* Returns the weight of the metric.
*/
public get weight(): number {
return 1;
}
weight: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

This now becomes a variable, not a getter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typescript-wise, this is the same. Public API wise too!


/**
* Adds a value to a metric.
*/
public add(value: number | string): void {
// Override this.
}
add(value: number | string): void;

/**
* Serializes the metric into a statsd format string.
*/
public toString(): string {
return '';
}
toString(): string;
}

export interface MetricBucketItem {
Expand Down