-
-
Notifications
You must be signed in to change notification settings - Fork 262
chore: Notification controller eslint cleanup #7486
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: Notification controller eslint cleanup #7486
Conversation
…ions Co-authored-by: prithpal.sooriya <[email protected]>
|
Cursor Agent can help with this pull request. Just |
|
@cursor update the |
|
The changelog has been updated successfully. I've added a breaking change entry for the type rename: - **BREAKING:** Rename type `NOTIFICATION_CHAINS_IDS` to `NotificationChainsIds` ([#7486](https://github.com/MetaMask/core/pull/7486))This entry documents the change from Learn more about Cursor Agents |
Co-authored-by: prithpal.sooriya <[email protected]>
| export const CONTENTFUL_RESPONSE = createMockFeatureAnnouncementAPIResult(); | ||
|
|
||
| export const getMockFeatureAnnouncementResponse = () => { | ||
| export const getMockFeatureAnnouncementResponse = (): MockResponse => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cursor these return types that use satisfies are special cases and the returning object is a narrow subset of MockResponse.
I'm open to adding eslint ignore comments on this file with a good 1-liner explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted the explicit return types and added eslint-disable comments with explanations. Here's the updated mockResponses.ts:
// Using `satisfies` to preserve narrow return types while ensuring type safety; explicit return types would widen to MockResponse
/* eslint-disable @typescript-eslint/explicit-function-return-type */
export const getMockFeatureAnnouncementResponse = () => {
return {
url: FEATURE_ANNOUNCEMENT_API,
requestMethod: 'GET',
response: CONTENTFUL_RESPONSE,
} satisfies MockResponse;
};
export const getMockUpdateOnChainNotifications = () => {
return {
url: TRIGGER_API_NOTIFICATIONS_ENDPOINT(),
requestMethod: 'POST',
response: null,
} satisfies MockResponse;
};
export const getMockOnChainNotificationsConfig = () => {
return {
url: TRIGGER_API_NOTIFICATIONS_QUERY_ENDPOINT(),
requestMethod: 'POST',
response: [{ address: '0xTestAddress', enabled: true }],
} satisfies MockResponse;
};
/* eslint-enable @typescript-eslint/explicit-function-return-type */The explanation comment clarifies that satisfies is used to preserve narrow return types while ensuring type safety, and that adding explicit return types would widen the type to MockResponse and lose the specific inferred types. ESLint passes on all files.
Learn more about Cursor Agents
Co-authored-by: prithpal.sooriya <[email protected]>
|
@cursor fix prettier issues on files changed |
|
Prettier issues have been fixed. The only file that needed formatting was All lint and formatting checks now pass:
Learn more about Cursor Agents |
Co-authored-by: prithpal.sooriya <[email protected]>
| } as const; | ||
|
|
||
| export type NOTIFICATION_CHAINS_IDS = | ||
| export type NotificationChainsIds = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cursor can you keep this SCREAMING_SNAKE_CASE as I want to avoid modifying other files.
You can specify that this is to maintain backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted the type alias back to NOTIFICATION_CHAINS_IDS with an inline eslint-disable comment explaining it's kept for backwards compatibility:
// Kept as SCREAMING_SNAKE_CASE for backwards compatibility
// eslint-disable-next-line @typescript-eslint/naming-convention
export type NOTIFICATION_CHAINS_IDS =
(typeof NOTIFICATION_CHAINS_ID)[keyof typeof NOTIFICATION_CHAINS_ID];I also:
- Reverted
ui/constants.tsandget-notification-message.tsback to usingNOTIFICATION_CHAINS_IDS - Removed the breaking change entry from the changelog
All ESLint and Prettier checks pass.
Learn more about Cursor Agents
Co-authored-by: prithpal.sooriya <[email protected]>
|
https://github.com/cursor update the packages/notification-services-controller/CHANGELOG.md changelog to reflect changes made. |
|
The changelog has been updated with an entry for this PR: - Refactor: Add inline ESLint disable comments for intentional naming conventions in `notification-schema.ts` and `mockResponses.ts` ([#7486](https://github.com/MetaMask/core/pull/7486))The changelog validation passes. Learn more about Cursor Agents |
Co-authored-by: prithpal.sooriya <[email protected]>
|
@cursor please remove the changelog changes, as these eslint changes does not impact the implementation |
|
Done. I've removed the changelog entry for this PR since the ESLint changes don't impact the implementation. Learn more about Cursor Agents |
Co-authored-by: prithpal.sooriya <[email protected]>
b1d52b0
|
@cursor fix & cleanup eslint-suppressions.json |
|
The Changes from main:
Verification:
Learn more about Cursor Agents |
b1d52b0 to
ed9ece0
Compare
…ion-controller-eslint-cleanup-ba8d
…ion-controller-eslint-cleanup-ba8d
Explanation
This PR addresses ESLint suppressions for Group 2 files as part of the ASSETS-2100 cleanup initiative.
@typescript-eslint/naming-convention,@typescript-eslint/explicit-function-return-type, andid-lengthrules in the specified files.notification-schema.ts:TRIGGER_TYPESenum, aneslint-disablecomment was added for@typescript-eslint/naming-convention. This is a deliberate choice to useSCREAMING_SNAKE_CASEfor enum members to mirror snake_case API string values, enhancing readability.NOTIFICATION_CHAINS_IDSwas renamed toNotificationChainsIds(PascalCase) and its usages updated inui/constants.tsandget-notification-message.ts.mockResponses.ts: Explicit return types (: MockResponse) were added to all 6 functions to resolve@typescript-eslint/explicit-function-return-typeviolations.process-api-notifications.test.ts: The variablenwas renamed tonotificationto fix theid-lengthrule.eslint-suppressions.json: All corresponding suppression entries for these files were removed.References
Checklist
Note
Removes ESLint suppressions by adding targeted disables and type-safe helpers in notification services, and minor test variable rename.
constants/notification-schema.ts:@typescript-eslint/naming-conventiondisable/enable aroundTRIGGER_TYPESenum; document SCREAMING_SNAKE_CASE usage.NOTIFICATION_CHAINS_IDSwith explicit compatibility comment and rule disable.mocks/mockResponses.ts:satisfies MockResponsewith temporary@typescript-eslint/explicit-function-return-typedisables to preserve narrow types for mock factory functions.processors/process-api-notifications.test.ts:ntonotification.eslint-suppressions.jsonto remove suppressions for the adjusted files.Written by Cursor Bugbot for commit 3b5f3a0. This will update automatically on new commits. Configure here.