bito-code-review[bot] commented on code in PR #43686:
URL: https://github.com/apache/superset/pull/43686#discussion_r3890295375
##########
superset-frontend/scripts/oxlint-metrics-uploader.js:
##########
@@ -280,8 +280,8 @@ async function runOxlintAndProcess() {
// Run the process, unless this file was imported (e.g. by a test) rather than
// executed, in which case nothing should be linted or uploaded on import.
-if (require.main === module) {
+if (__filename === process.argv[1]) {
runOxlintAndProcess().catch(console.error);
}
-module.exports = { parseRuleId };
+export default { parseRuleId };
Review Comment:
<!-- Bito Reply -->
The suggestion is correct and appropriate. In an ES module, `__filename` is
not available, and the proposed check using `import.meta.url` correctly ensures
the script only executes when run directly, preventing issues when the file is
imported.
**superset-frontend/scripts/oxlint-metrics-uploader.js**
```
import { pathToFileURL } from 'node:url';
// Run the process, unless this file was imported (e.g. by a test) rather
than
// executed, in which case nothing should be linted or uploaded on import.
if (import.meta.url === pathToFileURL(process.argv[1]).href) {
runOxlintAndProcess().catch(console.error);
}
```
##########
superset-frontend/scripts/internal/oxlint-metrics-uploader.js:
##########
@@ -0,0 +1,281 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { execSync } from 'node:child_process';
+import { GoogleAuth } from 'google-auth-library';
+import googleSheets from '@googleapis/sheets';
+
+const { SPREADSHEET_ID } = process.env;
+const SERVICE_ACCOUNT_KEY = JSON.parse(process.env.SERVICE_ACCOUNT_KEY ||
'{}');
+
+// Only set up Google Sheets if we have credentials
+let sheets;
+if (SERVICE_ACCOUNT_KEY.client_email) {
+ const auth = new GoogleAuth({
+ credentials: SERVICE_ACCOUNT_KEY,
+ scopes: ['https://www.googleapis.com/auth/spreadsheets'],
+ });
+ sheets = googleSheets.sheets({ version: 'v4', auth });
+}
+
+const DATETIME = new Date().toISOString().replace(/T/, ' ').replace(/\..+/,
'');
+
+/**
+ * Turn an oxlint diagnostic code into the canonical rule id used by the
metrics
+ * series.
+ *
+ * oxlint reports `<plugin>(<rule>)`, where the plugin is the linter the rule
came
+ * from: `eslint(no-console)`, `react-hooks(exhaustive-deps)`,
`react(jsx-key)`,
+ * `jest(no-conditional-expect)`, `oxc(erasing-op)`, and the legacy
+ * `eslint-plugin-unicorn(no-new-array)` spelling.
+ *
+ * `eslint` is the implicit namespace, so its rules keep their bare name and
stay
+ * comparable with the rows recorded before the oxlint migration. Every other
+ * plugin becomes `<plugin>/<rule>`, which is the id those rules are known by
in
+ * config and in the pre-migration history.
+ *
+ * @param {string | undefined} code the diagnostic's `code` field
+ * @returns {string} the rule id to record
+ */
+function parseRuleId(code) {
+ if (!code) {
+ return 'unknown';
+ }
+
+ const match = code.match(/^([\w-]+)\(([^)]+)\)$/);
+ if (!match) {
+ return code;
+ }
+
+ const [, namespace, rule] = match;
+ if (namespace === 'eslint') {
+ return rule;
+ }
+
+ // `eslint-plugin-unicorn(...)` is the same rule as `unicorn/...`
+ const plugin = namespace.replace(/^eslint-plugin-/, '');
+ return `${plugin}/${rule}`;
+}
+
+async function writeToGoogleSheet(data, range, headers, append = false) {
+ if (!sheets) {
+ console.log('No Google Sheets credentials, skipping upload');
+ return;
+ }
+
+ const request = {
+ spreadsheetId: SPREADSHEET_ID,
+ range,
+ valueInputOption: 'USER_ENTERED',
+ resource: { values: append ? data : [headers, ...data] },
+ };
+
+ const method = append ? 'append' : 'update';
+ await sheets.spreadsheets.values[method](request);
+}
+
+// Run OXC and get JSON output
+async function runOxlintAndProcess() {
+ const enrichedRules = {
+ 'react-prefer-function-component/react-prefer-function-component': {
+ description: 'We prefer function components to class-based components',
+ },
+ 'react/jsx-filename-extension': {
+ description:
+ 'We prefer Typescript - all JSX files should be converted to TSX',
+ },
+ 'react/forbid-component-props': {
+ description:
+ 'We prefer Emotion for styling rather than `className` or `style`
props',
+ },
+ 'no-restricted-imports': {
+ description:
+ "This rule catches several things that shouldn't be used anymore.
LESS, antD, etc. See individual occurrence messages for details",
+ },
+ 'no-console': {
+ description:
+ "We don't want a bunch of console noise, but you can use the `logger`
from `@superset-ui/core` when there's a reason to.",
+ },
+ };
+
+ try {
+ // Run OXC with JSON format
+ console.log('Running OXC linter...');
+ // `oxlint.json` is not the `.oxlintrc.json` oxlint auto-discovers, so the
+ // config has to be passed explicitly or the run reports oxlint's defaults
+ // instead of the project's ruleset. Matches the `lint` scripts in
+ // package.json.
+ const oxlintOutput = execSync(
+ 'npx oxlint --config oxlint.json --format json',
+ {
+ encoding: 'utf8',
+ maxBuffer: 50 * 1024 * 1024, // 50MB buffer for large outputs
+ stdio: ['pipe', 'pipe', 'ignore'], // Ignore stderr to avoid error
output
+ },
+ );
Review Comment:
<!-- Bito Reply -->
The observation regarding the existing try/catch structure is correct. The
current implementation only wraps the ESLint execution, leaving the oxlint
execution vulnerable to unhandled exceptions if it exits with a non-zero
status. Since this is a pre-existing pattern in the codebase, keeping the
review thread open for a separate follow-up fix is an appropriate approach.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
