tbonelee commented on code in PR #5279: URL: https://github.com/apache/zeppelin/pull/5279#discussion_r3544938044
########## zeppelin-web-angular/eslint-rules/ordered-exports.js: ########## @@ -0,0 +1,149 @@ +/* + * Licensed 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. + */ + +// ESLint reimplementation of the former custom TSLint `ordered-exports` rule +// (ZEPPELIN-6325). Enforces that top-level export statements -- typically the +// re-exports in a `public-api.ts` barrel -- are alphabetically ordered by their +// sort key (module specifier for `export ... from '...'`). ESLint replaces +// TSLint under ZEPPELIN-6372. + +'use strict'; + +/** + * Compute the alphabetical sort key for a top-level export statement, mirroring + * the original TSLint rule: module specifier for re-exports, otherwise the + * exported name(s) or declaration identifier. + * @param {import('eslint').Rule.Node} node + * @param {import('eslint').SourceCode} sourceCode + * @returns {string | null} + */ +function getSortKey(node, sourceCode) { + if (node.type === 'ExportAllDeclaration') { + return node.source ? node.source.value : null; + } + + if (node.type === 'ExportNamedDeclaration') { + if (node.source) { + return node.source.value; + } + if (node.declaration) { + const decl = node.declaration; + if (decl.type === 'VariableDeclaration') { + const first = decl.declarations[0]; + return first && first.id && first.id.name ? first.id.name : null; + } + return decl.id && decl.id.name ? decl.id.name : null; + } + if (node.specifiers && node.specifiers.length > 0) { + return node.specifiers.map(spec => spec.exported.name).join(', '); + } + return null; + } + + // `export default ...` and TypeScript `export = ...` are both ExportAssignment + // nodes in the TS compiler AST the original rule used, so both share this key. + if (node.type === 'ExportDefaultDeclaration' || node.type === 'TSExportAssignment') { + return 'export-assignment'; + } + + return null; +} + +function isExportStatement(node) { + return ( + node.type === 'ExportAllDeclaration' || + node.type === 'ExportNamedDeclaration' || + node.type === 'ExportDefaultDeclaration' || + node.type === 'TSExportAssignment' + ); +} + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + type: 'suggestion', + docs: { + description: 'Enforce alphabetically ordered top-level export statements (e.g. in public-api.ts barrels)' + }, + fixable: 'code', + schema: [ + { + type: 'object', + properties: { + // Optional: only apply to files whose path ends with one of these. + // When omitted the rule applies to every file it is configured for, + // so scoping is normally done via the flat-config `files` field. + targetFiles: { + type: 'array', + items: { type: 'string' } + } + }, + additionalProperties: false + } + ], + messages: { + unordered: 'Export statements should be alphabetically ordered by module specifier' + } + }, + + create(context) { + const options = context.options[0] || {}; + const targetFiles = Array.isArray(options.targetFiles) ? options.targetFiles : null; + const sourceCode = context.sourceCode || context.getSourceCode(); + + if (targetFiles && targetFiles.length > 0) { + const filename = context.filename || context.getFilename(); + if (!targetFiles.some(target => filename.endsWith(target))) { + return {}; + } + } + + return { + 'Program:exit'(program) { + const exports = []; + for (const node of program.body) { + if (!isExportStatement(node)) { + continue; + } + const sortKey = getSortKey(node, sourceCode); + if (sortKey !== null) { + exports.push({ node, sortKey }); + } + } + + if (exports.length <= 1) { + return; + } + + const compare = (a, b) => a.sortKey.localeCompare(b.sortKey, undefined, { sensitivity: 'base' }); + + const isSorted = exports.every((entry, i) => i === 0 || compare(exports[i - 1], entry) <= 0); + if (isSorted) { + return; + } + + const sorted = [...exports].sort(compare); + const first = exports[0].node; + const last = exports[exports.length - 1].node; + + context.report({ + node: first, + messageId: 'unordered', + fix(fixer) { + const fixText = sorted.map(entry => sourceCode.getText(entry.node).trim()).join('\n'); + return fixer.replaceTextRange([first.range[0], last.range[1]], fixText); Review Comment: This is the kind of edge case that makes me lean toward an upstream rule here (see my summary comment). The fix replaces the whole `[first.range[0], last.range[1]]` span with only the tracked export texts, so anything else in that span is deleted by `--fix`: ```ts export * from './zeta'; // --- widgets --- <- deleted export * from './alpha'; ``` The same happens to a non-export statement between exports, or an export whose sort key is `null` (e.g. `export {}`). Current barrels are clean so this is latent, but since lint-staged runs `--fix` on every commit, the first future occurrence would be silent code loss. If we keep the custom rule, preserving the full text between statements (slicing from each statement's start to the next statement's start, like the TSLint `getFullStart()` approach) plus a regression test would cover it. ########## zeppelin-web-angular/eslint-rules/ordered-exports.js: ########## @@ -0,0 +1,149 @@ +/* + * Licensed 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. + */ + +// ESLint reimplementation of the former custom TSLint `ordered-exports` rule +// (ZEPPELIN-6325). Enforces that top-level export statements -- typically the +// re-exports in a `public-api.ts` barrel -- are alphabetically ordered by their +// sort key (module specifier for `export ... from '...'`). ESLint replaces +// TSLint under ZEPPELIN-6372. + +'use strict'; + +/** + * Compute the alphabetical sort key for a top-level export statement, mirroring + * the original TSLint rule: module specifier for re-exports, otherwise the + * exported name(s) or declaration identifier. + * @param {import('eslint').Rule.Node} node + * @param {import('eslint').SourceCode} sourceCode + * @returns {string | null} + */ +function getSortKey(node, sourceCode) { + if (node.type === 'ExportAllDeclaration') { + return node.source ? node.source.value : null; + } + + if (node.type === 'ExportNamedDeclaration') { + if (node.source) { + return node.source.value; + } + if (node.declaration) { + const decl = node.declaration; + if (decl.type === 'VariableDeclaration') { + const first = decl.declarations[0]; + return first && first.id && first.id.name ? first.id.name : null; + } + return decl.id && decl.id.name ? decl.id.name : null; + } + if (node.specifiers && node.specifiers.length > 0) { + return node.specifiers.map(spec => spec.exported.name).join(', '); + } + return null; + } + + // `export default ...` and TypeScript `export = ...` are both ExportAssignment + // nodes in the TS compiler AST the original rule used, so both share this key. + if (node.type === 'ExportDefaultDeclaration' || node.type === 'TSExportAssignment') { Review Comment: Minor note: `export default class Foo {}` was a ClassDeclaration in the TSLint rule (sorted by name `Foo`); only `export default <expr>` and `export =` were ExportAssignment. Here every `ExportDefaultDeclaration` gets the fixed `'export-assignment'` key. Harmless today since no barrel uses `export default`, but it also means we're not strictly bound to the old sort semantics, which makes switching to an upstream rule easier if we go that way. ########## zeppelin-web-angular/eslint-rules/constructor-params-order.js: ########## @@ -0,0 +1,157 @@ +/* + * Licensed 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. + */ + +// ESLint reimplementation of the former custom TSLint `constructor-params-order` +// rule (ZEPPELIN-6301). Enforces the constructor parameter order +// public -> protected -> private (-> none -> optional) for a consistent New UI +// constructor style. ESLint replaces TSLint under ZEPPELIN-6372. + +'use strict'; + +const RANK = { public: 0, protected: 1, private: 2, none: 3, optional: 4 }; + +/** + * Collect decorators attached to a (possibly parameter-property) parameter node. + * @param {any} param + * @returns {any[]} + */ +function getDecorators(param) { + if (Array.isArray(param.decorators) && param.decorators.length > 0) { + return param.decorators; + } + // TSParameterProperty wraps the real parameter in `.parameter`. + if (param.parameter && Array.isArray(param.parameter.decorators)) { + return param.parameter.decorators; + } + return []; +} + +/** + * True when the parameter is optional -- either annotated with `@Optional()` + * (Angular DI) or declared with a `?` question token. + * @param {any} param + * @returns {boolean} + */ +function isOptional(param) { + const hasOptionalDecorator = getDecorators(param).some(decorator => { + let expr = decorator.expression; + if (expr && expr.type === 'CallExpression') { + expr = expr.callee; + } + return expr && expr.type === 'Identifier' && expr.name === 'Optional'; + }); + if (hasOptionalDecorator) { + return true; + } + const inner = param.type === 'TSParameterProperty' ? param.parameter : param; + return !!(inner && inner.optional); +} + +/** + * Map a constructor parameter to its accessibility rank. + * @param {any} param + * @returns {number} + */ +function getRank(param) { + if (isOptional(param)) { + return RANK.optional; + } + if (param.type === 'TSParameterProperty' && param.accessibility) { + return RANK[param.accessibility]; + } + return RANK.none; +} + +/** + * Full source range of a parameter, extended backward to include leading + * decorators and leading comments so that an inter-parameter comment travels + * with its parameter when the fixer reorders them. The original TSLint rule + * sliced from getFullStart() (leading trivia included) for the same reason. + * @param {any} param + * @param {import('eslint').SourceCode} sourceCode + * @returns {[number, number]} + */ +function getParamRange(param, sourceCode) { + let start = param.range[0]; + let leadNode = param; + for (const decorator of getDecorators(param)) { + if (decorator.range[0] < start) { + start = decorator.range[0]; + leadNode = decorator; + } + } + const comments = sourceCode.getCommentsBefore(leadNode); + if (comments.length > 0 && comments[0].range[0] < start) { + start = comments[0].range[0]; + } + return [start, param.range[1]]; +} + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + type: 'suggestion', + docs: { + description: 'Enforce constructor parameter order: public, protected, private' + }, + fixable: 'code', + schema: [], + messages: { + order: 'Constructor parameters should be ordered: public, protected, private' + } + }, + + create(context) { + const sourceCode = context.sourceCode || context.getSourceCode(); + + /** + * @param {any} node MethodDefinition with kind === 'constructor' + */ + function checkConstructor(node) { + const params = node.value.params; + if (!params || params.length <= 1) { + return; + } + + const ranks = params.map(getRank); + const inOrder = ranks.every((rank, i) => i === 0 || ranks[i - 1] <= rank); + if (inOrder) { + return; + } + + context.report({ + node, + messageId: 'order', + fix(fixer) { + const ranges = params.map(p => getParamRange(p, sourceCode)); + // Stable sort by rank preserves the original relative order within a + // group, matching the previous TSLint behaviour. + const order = params.map((_, i) => i).sort((a, b) => ranks[a] - ranks[b]); + + const firstStart = ranges[0][0]; + const lastEnd = ranges[params.length - 1][1]; + const isMultiLine = sourceCode.getText().slice(firstStart, lastEnd).includes('\n'); + const indent = ' '.repeat(params[0].loc.start.column); + + const texts = order.map(i => sourceCode.getText().slice(ranges[i][0], ranges[i][1]).trim()); + const joined = isMultiLine ? texts.join(`,\n${indent}`) : texts.join(', '); + + return fixer.replaceTextRange([firstStart, lastEnd], joined); Review Comment: I ran the rule to check a hunch about comment handling: `constructor(private b: B /* injected lazily */, public a: A)` fixes to `constructor(public a: A, private b: B)`, deleting the comment. `getParamRange(b)` ends at `b.range[1]` and `getCommentsBefore(a)` stops at the comma token, so the comment falls inside the replaced range but in no param's text. Relatedly, a trailing same-line comment (`private a: A, // about a`) travels to the *next* param after reordering. The original TSLint fixer handled both by slicing `getFullStart()`-to-`getFullStart()`; porting that strategy would fix both cases and also removes the need for the indent recomputation below. ########## zeppelin-web-angular/eslint-rules/constructor-params-order.js: ########## @@ -0,0 +1,157 @@ +/* + * Licensed 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. + */ + +// ESLint reimplementation of the former custom TSLint `constructor-params-order` +// rule (ZEPPELIN-6301). Enforces the constructor parameter order +// public -> protected -> private (-> none -> optional) for a consistent New UI +// constructor style. ESLint replaces TSLint under ZEPPELIN-6372. + +'use strict'; + +const RANK = { public: 0, protected: 1, private: 2, none: 3, optional: 4 }; + +/** + * Collect decorators attached to a (possibly parameter-property) parameter node. + * @param {any} param + * @returns {any[]} + */ +function getDecorators(param) { + if (Array.isArray(param.decorators) && param.decorators.length > 0) { + return param.decorators; + } + // TSParameterProperty wraps the real parameter in `.parameter`. + if (param.parameter && Array.isArray(param.parameter.decorators)) { + return param.parameter.decorators; + } + return []; +} + +/** + * True when the parameter is optional -- either annotated with `@Optional()` + * (Angular DI) or declared with a `?` question token. + * @param {any} param + * @returns {boolean} + */ +function isOptional(param) { + const hasOptionalDecorator = getDecorators(param).some(decorator => { + let expr = decorator.expression; + if (expr && expr.type === 'CallExpression') { + expr = expr.callee; + } + return expr && expr.type === 'Identifier' && expr.name === 'Optional'; + }); + if (hasOptionalDecorator) { + return true; + } + const inner = param.type === 'TSParameterProperty' ? param.parameter : param; + return !!(inner && inner.optional); +} + +/** + * Map a constructor parameter to its accessibility rank. + * @param {any} param + * @returns {number} + */ +function getRank(param) { + if (isOptional(param)) { + return RANK.optional; + } + if (param.type === 'TSParameterProperty' && param.accessibility) { + return RANK[param.accessibility]; + } + return RANK.none; +} + +/** + * Full source range of a parameter, extended backward to include leading + * decorators and leading comments so that an inter-parameter comment travels + * with its parameter when the fixer reorders them. The original TSLint rule + * sliced from getFullStart() (leading trivia included) for the same reason. + * @param {any} param + * @param {import('eslint').SourceCode} sourceCode + * @returns {[number, number]} + */ +function getParamRange(param, sourceCode) { + let start = param.range[0]; + let leadNode = param; + for (const decorator of getDecorators(param)) { + if (decorator.range[0] < start) { + start = decorator.range[0]; + leadNode = decorator; + } + } + const comments = sourceCode.getCommentsBefore(leadNode); + if (comments.length > 0 && comments[0].range[0] < start) { + start = comments[0].range[0]; + } + return [start, param.range[1]]; +} + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { Review Comment: Could you add a `RuleTester` suite for this rule (valid/invalid/`output` cases, including the comment and multi-line scenarios noted in the other comments)? It runs under plain Node with the already-installed deps, and since this fixer rewrites source files unattended on every commit, having regression coverage for the tricky cases would make future changes to it much safer. ########## zeppelin-web-angular/eslint-rules/constructor-params-order.js: ########## @@ -0,0 +1,157 @@ +/* + * Licensed 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. + */ + +// ESLint reimplementation of the former custom TSLint `constructor-params-order` +// rule (ZEPPELIN-6301). Enforces the constructor parameter order +// public -> protected -> private (-> none -> optional) for a consistent New UI +// constructor style. ESLint replaces TSLint under ZEPPELIN-6372. + +'use strict'; + +const RANK = { public: 0, protected: 1, private: 2, none: 3, optional: 4 }; + +/** + * Collect decorators attached to a (possibly parameter-property) parameter node. + * @param {any} param + * @returns {any[]} + */ +function getDecorators(param) { + if (Array.isArray(param.decorators) && param.decorators.length > 0) { + return param.decorators; + } + // TSParameterProperty wraps the real parameter in `.parameter`. + if (param.parameter && Array.isArray(param.parameter.decorators)) { + return param.parameter.decorators; + } + return []; +} + +/** + * True when the parameter is optional -- either annotated with `@Optional()` + * (Angular DI) or declared with a `?` question token. + * @param {any} param + * @returns {boolean} + */ +function isOptional(param) { + const hasOptionalDecorator = getDecorators(param).some(decorator => { + let expr = decorator.expression; + if (expr && expr.type === 'CallExpression') { + expr = expr.callee; + } + return expr && expr.type === 'Identifier' && expr.name === 'Optional'; + }); + if (hasOptionalDecorator) { + return true; + } + const inner = param.type === 'TSParameterProperty' ? param.parameter : param; + return !!(inner && inner.optional); +} + +/** + * Map a constructor parameter to its accessibility rank. + * @param {any} param + * @returns {number} + */ +function getRank(param) { + if (isOptional(param)) { + return RANK.optional; + } + if (param.type === 'TSParameterProperty' && param.accessibility) { + return RANK[param.accessibility]; + } + return RANK.none; +} + +/** + * Full source range of a parameter, extended backward to include leading + * decorators and leading comments so that an inter-parameter comment travels + * with its parameter when the fixer reorders them. The original TSLint rule + * sliced from getFullStart() (leading trivia included) for the same reason. + * @param {any} param + * @param {import('eslint').SourceCode} sourceCode + * @returns {[number, number]} + */ +function getParamRange(param, sourceCode) { + let start = param.range[0]; + let leadNode = param; + for (const decorator of getDecorators(param)) { + if (decorator.range[0] < start) { + start = decorator.range[0]; + leadNode = decorator; + } + } + const comments = sourceCode.getCommentsBefore(leadNode); + if (comments.length > 0 && comments[0].range[0] < start) { + start = comments[0].range[0]; + } + return [start, param.range[1]]; +} + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + type: 'suggestion', + docs: { + description: 'Enforce constructor parameter order: public, protected, private' + }, + fixable: 'code', + schema: [], + messages: { + order: 'Constructor parameters should be ordered: public, protected, private' + } + }, + + create(context) { + const sourceCode = context.sourceCode || context.getSourceCode(); + + /** + * @param {any} node MethodDefinition with kind === 'constructor' + */ + function checkConstructor(node) { + const params = node.value.params; + if (!params || params.length <= 1) { + return; + } + + const ranks = params.map(getRank); + const inOrder = ranks.every((rank, i) => i === 0 || ranks[i - 1] <= rank); + if (inOrder) { + return; + } + + context.report({ + node, + messageId: 'order', + fix(fixer) { + const ranges = params.map(p => getParamRange(p, sourceCode)); + // Stable sort by rank preserves the original relative order within a + // group, matching the previous TSLint behaviour. + const order = params.map((_, i) => i).sort((a, b) => ranks[a] - ranks[b]); + + const firstStart = ranges[0][0]; + const lastEnd = ranges[params.length - 1][1]; + const isMultiLine = sourceCode.getText().slice(firstStart, lastEnd).includes('\n'); + const indent = ' '.repeat(params[0].loc.start.column); Review Comment: Nit: when the first param shares a line with `constructor(`, the computed indent is the first param's column (14+ spaces). Prettier re-normalizes it in the lint-staged and `lint:fix` pipelines, but a standalone editor `eslint --fix` shows it. This goes away automatically with the full-slice fixer strategy above. ########## zeppelin-web-angular/eslint.config.js: ########## @@ -146,6 +155,15 @@ module.exports = tseslint.config( '@angular-eslint/directive-selector': ['error', { type: 'attribute', prefix: 'lib', style: 'camelCase' }] } }, + { + // Custom rule (reimplemented from TSLint, ZEPPELIN-6372): keep public-api.ts + // barrels alphabetically ordered by module specifier. + files: ['**/public-api.ts'], + plugins: { local: localRules }, Review Comment: Since `public-api.ts` files also match the main `**/*.ts` block, this registers the `local` namespace twice. ESLint accepts that only while both entries are the exact same object reference, so a future refactor that breaks the identity (a spread, a second `require` from another path) would fail every `public-api.ts` lint with "Cannot redefine plugin". My suggestion: hoist the registration into a dedicated `files`-less config object at the top, which applies to all linted files: ```js { // register the local plugin once, for every file plugins: { local: localRules } }, ``` Then both this block and the main `**/*.ts` block only carry `rules`, each stays independent of the others' globs, and there's a single registration site. -- 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]
