Re: [PR] refactor: move translations and logging to new core [superset]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #36929:
URL: https://github.com/apache/superset/pull/36929#discussion_r2676877595


##########
superset-frontend/packages/superset-core/src/ui/translation/Translator.ts:
##########
@@ -17,7 +17,7 @@
  * under the License.
  */
 import UntypedJed from 'jed';
-import logging from '../utils/logging';
+import logging from '../../utils/logging';

Review Comment:
   The import path uses relative paths across multiple directory levels (`../../`). Consider using an absolute import from the package root for better maintainability and clarity.
   ```suggestion
   import logging from 'src/utils/logging';
   ```



##########
superset-frontend/packages/superset-ui-core/test/utils/featureFlag.test.ts:
##########
@@ -51,7 +56,7 @@ test('returns false and raises console error if feature flags have not been init
   Object.defineProperty(window, 'featureFlags', {
     value: undefined,
   });
-  expect(uiCore.isFeatureEnabled(uiCore.FeatureFlag.DrillBy)).toEqual(false);
+  expect(isFeatureEnabled(FeatureFlag.DrillBy)).toEqual(false);
   expect(uiCore.logging.error).toHaveBeenCalled();
   expect(logging).toHaveBeenCalledWith('Failed to query feature flag DRILL_BY');

Review Comment:
   Line 61 references an undefined variable `logging`. This should be `uiCore.logging.error` to match the mock being tested on line 60.
   ```suggestion
     expect(uiCore.logging.error).toHaveBeenCalledWith('Failed to query feature flag DRILL_BY');
   ```



##########
superset-frontend/packages/superset-core/src/utils/logging.ts:
##########
@@ -31,7 +31,7 @@ const logger = {
 };
 
 /**
- * Superset frontend logger, currently just an alias to console.
+ * Superset logger, currently just an alias to console.

Review Comment:
   The comment was updated from 'Superset frontend logger' to 'Superset logger', but this is still in the frontend package. The comment should clarify that this is for frontend logging to avoid confusion.
   ```suggestion
    * Superset frontend logger, currently just an alias to the browser console.
   ```



##########
superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/ThemedAgGridReact.test.tsx:
##########
@@ -20,12 +20,12 @@ import { render, screen } from '@superset-ui/core/spec';
 import { AgGridReact } from 'ag-grid-react';
 import { createRef } from 'react';
 import { ThemeProvider, supersetTheme } from '@apache-superset/core';
-import * as themeUtils from '@apache-superset/core/ui/theme/utils/themeUtils';
+import * as uiModule from '@apache-superset/core/ui';
 import { ThemedAgGridReact } from './index';
 
 // Mock useThemeMode hook
-jest.mock('@apache-superset/core/ui/theme/utils/themeUtils', () => ({
-  ...jest.requireActual('@apache-superset/core/ui/theme/utils/themeUtils'),
+jest.mock('@apache-superset/core/ui', () => ({
+  ...jest.requireActual('@apache-superset/core/ui'),

Review Comment:
   The mock is now targeting the entire `@apache-superset/core/ui` module instead of the more specific `theme/utils/themeUtils` path. This broader mock could hide issues with other exports from the ui module. Consider maintaining the more specific mock target if possible.
   ```suggestion
   jest.mock('@apache-superset/core/ui/theme/utils/themeUtils', () => ({
     ...jest.requireActual('@apache-superset/core/ui/theme/utils/themeUtils'),
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org