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.

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

fix(eslint): Enable no-constant-binary-expression and fix violations #83273

Merged
merged 4 commits into from
Jan 14, 2025
Merged

fix(eslint): Enable no-constant-binary-expression and fix violations #83273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a388d3e
fix(eslint): Enable no-constant-binary-expression and fix violations
ryan953 Jan 10, 2025
e58d63b
remove the rule from eslint.config
ryan953 Jan 10, 2025
5d7fc9d
use correct DataCategoryExact.SPAN_INDEXED enum
ryan953 Jan 10, 2025
d23eff1
also enable valid-typeof
ryan953 Jan 13, 2025
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
2 changes: 0 additions & 2 deletions eslint.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,6 @@ export default typescript.config([
'no-cond-assign': ['error', 'always'],
'no-async-promise-executor': 'off', // TODO(ryan953): Fix violations and delete this line
'no-case-declarations': 'off', // TODO(ryan953): Fix violations and delete this line
'no-constant-binary-expression': 'off', // TODO(ryan953): Fix violations and delete this line
'no-dupe-class-members': 'off', // TODO(ryan953): Fix violations and delete this line
'no-dupe-else-if': 'off', // TODO(ryan953): Fix violations and delete this line
'no-empty-pattern': 'off', // TODO(ryan953): Fix violations and delete this line
Expand All @@ -296,7 +295,6 @@ export default typescript.config([
'no-unused-vars': 'off', // TODO(ryan953): Fix violations and delete this line
'no-useless-catch': 'off', // TODO(ryan953): Fix violations and delete this line
'no-useless-escape': 'off', // TODO(ryan953): Fix violations and delete this line
'valid-typeof': 'off', // TODO(ryan953): Fix violations and delete this line
},
},
{
Expand Down
7 changes: 3 additions & 4 deletions static/app/components/charts/percentageAreaChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,9 @@ export default class PercentageAreaChart extends Component<Props> {
const series = toArray(seriesParams);

// Filter series that have 0 counts
const date =
`${
series.length && moment(series[0].data[0]).format('MMM D, YYYY')
}<br />` || '';
const date = `${
series.length && moment(series[0].data[0]).format('MMM D, YYYY')
}<br />`;

return [
'<div class="tooltip-series">',
Expand Down
12 changes: 6 additions & 6 deletions static/app/utils/profiling/profile/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ export function createContinuousProfileFrameIndex(

for (let i = 0; i < frames.length; i++) {
const frame = frames[i]!;
const frameKey = `${frame.filename ?? ''}:${frame.function ?? 'unknown'}:${
String(frame.lineno) ?? ''
}:${frame.instruction_addr ?? ''}`;
const frameKey = `${frame.filename ?? ''}:${frame.function ?? 'unknown'}:${String(
frame.lineno
)}:${frame.instruction_addr ?? ''}`;

const existing = insertionCache[frameKey];
if (existing) {
Expand Down Expand Up @@ -64,9 +64,9 @@ export function createSentrySampleProfileFrameIndex(

for (let i = 0; i < frames.length; i++) {
const frame = frames[i]!;
const frameKey = `${frame.filename ?? ''}:${frame.function ?? 'unknown'}:${
String(frame.lineno) ?? ''
}:${frame.instruction_addr ?? ''}`;
const frameKey = `${frame.filename ?? ''}:${frame.function ?? 'unknown'}:${String(
frame.lineno
)}:${frame.instruction_addr ?? ''}`;

const existing = insertionCache[frameKey];
if (existing) {
Expand Down
6 changes: 1 addition & 5 deletions static/app/views/performance/utils/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,11 +369,7 @@ export function getProject(
eventData: EventData,
projects: Project[]
): Project | undefined {
const projectSlug = (eventData?.project as string) || undefined;

if (typeof projectSlug === undefined) {
Copy link
Member Author

Choose a reason for hiding this comment

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

typeof returns a string, it can never equal undefined... but it could equal "undefined"

Copy link
Member Author

Choose a reason for hiding this comment

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

both no-constant-binary-expression and valid-typeof would catch this statement.

return undefined;
}
const projectSlug = eventData.project as string | undefined;

return projects.find(currentProject => currentProject.slug === projectSlug);
}
Expand Down
5 changes: 3 additions & 2 deletions static/app/views/profiling/profileSummary/regressedProfileFunctions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,9 @@ export function MostRegressedProfileFunctions(props: MostRegressedProfileFunctio

const onChangeTrendType = useCallback(v => setTrendType(v.value), []);

const hasDifferentialFlamegraphPageFeature =
false && organization.features.includes('profiling-differential-flamegraph-page');
const hasDifferentialFlamegraphPageFeature = organization.features.includes(
'profiling-differential-flamegraph-page'
);
Comment on lines +165 to +167
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @JonasBa @Zylphrex you're the only two mentioned in the flagpole config for this. It'll be enabled now for ya.


return (
<RegressedFunctionsContainer>
Expand Down
17 changes: 8 additions & 9 deletions static/app/views/settings/account/notifications/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,21 @@ export function getDocsLinkForEventType(
event: DataCategoryExact | string // TODO(isabella): get rid of strings after removing need for backward compatibility on gs
) {
switch (event) {
case DataCategoryExact.TRANSACTION || 'transaction':
Copy link
Member Author

@ryan953 ryan953 Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This should resolve to be DataCategoryExact.TRANSACTION || 'transaction' === DataCategoryExact.TRANSACTION therefore the right hand side of the || isn't needed.

It doesn't really matter because the enum value and the string value are the same, so this is a true statement: DataCategoryExact.TRANSACTION === 'transaction'. We don't need to write it out twice, no matter what the function types are.

case DataCategoryExact.TRANSACTION:
// For pre-AM3 plans prior to June 11th, 2024
return 'https://docs.sentry.io/pricing/quotas/legacy-manage-transaction-quota/';
case DataCategoryExact.SPAN ||
DataCategoryExact.SPAN_INDEXED ||
'span' ||
'span_indexed':
Comment on lines -47 to -50
Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't working before when the function was called with DataCategoryExact.SPAN_INDEXED or span_indexed.

The case would boil down to be the same as case DataCategoryExact.SPAN only so values like "span_indexed" and "spanIndexed" wouldn't match at all and would fall through to the default case below.

Can play with it here: https://www.typescriptlang.org/play/?#code/GYVwdgxgLglg9mABFOA5EBbARgUwE4AUMYADiFAFyIDOUexA5gJSIDeAUO4jQO4xQQAFkVLkWHbtwgBDajkQByBDgUUuk7nhxQQeJAEYA3Jw0y5iqDzgLEAH1uKAJnGqr1GrTr2IATMfeIjjjA0iAANpQBmtq6BgAMccbcAL7sqewQCNRwYTgAdGFwDARKYCoANMhomLiEpSpMTMaZYNm5BUUlltaVKOjY+F1WCo3NWTn5hcUKzq691QN1syNNQA

SCR-20250110-mqxy

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @isabellaenriquez

case DataCategoryExact.SPAN:
case DataCategoryExact.SPAN_INDEXED:
case 'span_indexed':
// For post-AM3 plans after June 11th, 2024
return 'https://docs.sentry.io/pricing/quotas/manage-transaction-quota/';
case DataCategoryExact.ATTACHMENT || 'attachment':
case DataCategoryExact.ATTACHMENT:
return 'https://docs.sentry.io/product/accounts/quotas/manage-attachments-quota/#2-rate-limiting';
case DataCategoryExact.REPLAY || 'replay':
case DataCategoryExact.REPLAY:
return 'https://docs.sentry.io/product/session-replay/';
case DataCategoryExact.MONITOR_SEAT || 'monitorSeat':
case DataCategoryExact.MONITOR_SEAT:
return 'https://docs.sentry.io/product/crons/';
case DataCategoryExact.PROFILE_DURATION || 'profileDuration':
case DataCategoryExact.PROFILE_DURATION:
return 'https://docs.sentry.io/product/explore/profiling/';
default:
return 'https://docs.sentry.io/product/accounts/quotas/manage-event-stream-guide/#common-workflows-for-managing-your-event-stream';
Expand Down
24 changes: 11 additions & 13 deletions static/app/views/traces/content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,17 @@ function Content() {
const handleClearSearch = useCallback(
(searchIndex: number) => {
const newQueries = [...queries];
if (typeof newQueries[searchIndex] !== undefined) {
delete newQueries[searchIndex];
browserHistory.push({
...location,
query: {
...location.query,
cursor: undefined,
query: newQueries,
},
});
return true;
}
return false;
// TODO: do we need to return false when `newQueries[searchIndex] === undefined`?
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to 'fix' the existing if-statement because that would cause a change in behavior. But if we need that guard to exist, then it only needs to be if (newQueries[searchIndex] === undefined) { ...

delete newQueries[searchIndex];
browserHistory.push({
...location,
query: {
...location.query,
cursor: undefined,
query: newQueries,
},
});
return true;
},
[location, queries]
);
Expand Down
3 changes: 1 addition & 2 deletions static/app/views/traces/tracesChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,7 @@ export function TracesChart({}: Props) {
}
const data = series[i]!['count()'];
data.color = CHART_PALETTE[2][i];
data.seriesName =
`span ${i + 1}: ${queries[i] || t('All spans')}` || t('All spans');
data.seriesName = `span ${i + 1}: ${queries[i] || t('All spans')}`;
allData.push(data);
}

Expand Down
Loading