Add 'generate-index' function to GLSP CLI #1197

martin-fleck-at · 2023-12-15T16:11:15Z

No description provided.

tortmayr

In general, this utility command looks good to me and is very helpful.
However, we have to be careful with this. We have a couple of cases where we
deliberately exclude files from the index. (e.g. test-util in @eclipse-glsp/protocol
to avoid a direct chai dependency).

Currently the tooling cannot handle these cases and would simply overrwrite (and therefore export) the index file.
I think we need a blacklist approach here.

martin-fleck-at · 2024-03-22T09:02:25Z

@tortmayr That is a very good point! I see that globby is a package that offers a lot of flexibility in this regard, most notably custom 'ignore' files (e.g., .indexignore). I used this to add the required functionality. However, to use it more nicely, I had to convert the package into an ES Module. Could you please have a look whether that is alright with you as well?

tortmayr · 2024-03-23T11:10:06Z

Unfortunatley the CI does not seem to be happy with the mix of commonjs and es modules:

 Error [ERR_REQUIRE_ESM]: require() of ES Module /home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/dev-packages/cli/.eslintrc.js from /home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/@eslint/eslintrc/dist/eslintrc.cjs not supported.
@eclipse-glsp/cli: .eslintrc.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
@eclipse-glsp/cli: Instead rename .eslintrc.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/dev-packages/cli/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).
@eclipse-glsp/cli:     at Object.module.exports [as default] (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/import-fresh/index.js:32:59)
@eclipse-glsp/cli:     at loadJSConfigFile (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2574:47)
@eclipse-glsp/cli:     at loadConfigFile (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2658:20)
@eclipse-glsp/cli:     at ConfigArrayFactory.loadInDirectory (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2868:34)
@eclipse-glsp/cli:     at CascadingConfigArrayFactory._loadConfigInAncestors (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3860:46)
@eclipse-glsp/cli:     at CascadingConfigArrayFactory._loadConfigInAncestors (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3879:20)
@eclipse-glsp/cli:     at CascadingConfigArrayFactory.getConfigArrayForFile (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3781:18)
@eclipse-glsp/cli:     at FileEnumerator._iterateFilesRecursive (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/eslint/lib/cli-engine/file-enumerator.js:450:49)
@eclipse-glsp/cli:     at _iterateFilesRecursive.next (<anonymous>)
@eclipse-glsp/cli:     at FileEnumerator.iterateFiles (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/eslint/lib/cli-engine/file-enumerator.js:299:49)
@eclipse-glsp/cli:     at iterateFiles.next (<anonymous>)
@eclipse-glsp/cli:     at CLIEngine.executeOnFiles (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/eslint/lib/cli-engine/cli-engine.js:797:48)
@eclipse-glsp/cli:     at ESLint.lintFiles (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/eslint/lib/eslint/eslint.js:551:23)
@eclipse-glsp/cli:     at Object.execute (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/eslint/lib/cli.js:395:36)
@eclipse-glsp/cli:     at async main (/home/jenkins/agent/workspace/eclipse-glsp_glsp_PR-1197/node_modules/eslint/bin/eslint.js:146:24)

- Turn CLI package into ES Module so we can easily use 'globby' - Minor fix in 'check-headers' if second line does not contain year

martin-fleck-at · 2024-04-15T07:55:15Z

@tortmayr I pushed an update that should fix that problem.

tortmayr

Thanks seems to work as expected now. The .indexignore file is a neat new feature.
One other issue that I noticed. When recreating index files the copyright header will be removed.
e.g. After executing glsp generateIndex glsp-client/packages/protocol/src -f all index file have linting errors due to the missing header.

Also it would be nice if the root dir option would be variadic so that we can generate index file in all relevant packages with one command. This can be done by adding ... to the argument:

 .argument('<rootDirs...>', 'The source directories for index generation.')
 ...
 export async function generateIndex(rootDirs: string[], options: GenerateIndexCmdOptions): Promise<void> {

In addition, we should probably preprocess the received rootDirs with the validateDirectory utility function to ensure that
the directories exist and are resolved to the absolute path

  rootDirs
       .map(rootDir => path.resolve(rootDir))

martin-fleck-at · 2024-04-16T10:53:19Z

@tortmayr Good ideas! I added a new commit that should hopefully solve all that problems.

tortmayr

Thanks for the fast update. I had a look at the code again and have some additional comments. Sorry for being nitpicky 😉.

tortmayr · 2024-04-16T11:27:04Z

dev-packages/cli/src/commands/generate-index.ts

+}
+
+export function verbose(options: GenerateIndexCmdOptions, message?: any, ...optionalParams: any[]): void {
+    if (options.verbose) {


Any particular reason why you dont use the LOGGER` utility we already have in place for this?

Just configure the logger in your entry function with

configureLogger(options.verbose); // and use it with LOGGER.info() // for non-verbose logging LOGGER.debug() // for verbose logging

see also (

glsp/dev-packages/cli/src/commands/update-next.ts

Line 35 in b5aff4f

configureLogger(options.verbose);

)

The reason is that I just wasn't properly aware of it, I'll fix it.

tortmayr · 2024-04-16T11:44:12Z

dev-packages/cli/src/commands/generate-index.ts

+
+export function extractHeader(fileContent: string): string[] {
+    // any and all comments before actual content are considered as part of the header
+    const comments: string[] = [];


I don't think its necessary to complex header comment parsing here.
We could simplify this and just keep all line above the first export.
So we could just read the file, use a regex matching everything starting from the first export (e.g /export\b.*/s;) and then replace the old exports with the newly generated ones.

tortmayr · 2024-04-16T11:50:17Z

Also the copyright headers are now polluting the log output

Overwrite index file /home/tobias/Git/OpenSource/glsp/glsp-client/packages/client/src/utils/index.ts
 * Copyright (c) 2023 EclipseSource and others.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License v. 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0.
 *
 * This Source Code may also be made available under the following Secondary
 * Licenses when the conditions for such availability set forth in the Eclipse
 * Public License v. 2.0 are satisfied: GNU General Public License, version 2
 * with the GNU Classpath Exception which is available at
 * https://www.gnu.org/software/classpath/license.html.
 *
 * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
 ********************************************************************************/
Overwrite index file /home/tobias/Git/OpenSource/glsp/glsp-client/packages/client/src/views/index.ts
 * Copyright (c) 2023 EclipseSource and others.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License v. 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0.
 *
 * This Source Code may also be made available under the following Secondary
 * Licenses when the conditions for such availability set forth in the Eclipse
 * Public License v. 2.0 are satisfied: GNU General Public License, version 2
 * with the GNU Classpath Exception which is available at
 * https://www.gnu.org/software/classpath/license.html.
 *
 * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
 ********************************************************************************/
Overwrite index file /home/tobias/Git/OpenSource/glsp/glsp-client/packages/client/src/base/index.ts
 * Copyright (c) 2023 EclipseSource and others.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License v. 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0.
 *
 * This Source Code may also be made available under the following Secondary
 * Licenses when the conditions for such availability set forth in the Eclipse
 * Public License v. 2.0 are satisfied: GNU General Public License, version 2
 * with the GNU Classpath Exception which is available at
 * https://www.gnu.org/software/classpath/license.html.
 *
 * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
 ********************************************************************************/
Overwrite index file /home/tobias/Git/OpenSource/glsp/glsp-client/packages/client/src/index.ts

It would be cleaner to not print them (at least in non-verbose mode)

martin-fleck-at · 2024-04-16T13:40:17Z

@tortmayr Thank you for your quick feedback, the additional logging was indeed an oversight. I pushed an update.

tortmayr

Thanks. Everything looks good to me now! 👍🏼

martin-fleck-at requested a review from tortmayr December 15, 2023 16:11

tortmayr reviewed Mar 17, 2024

View reviewed changes

martin-fleck-at force-pushed the generate-index branch 2 times, most recently from f2c91ed to 94d90bc Compare March 22, 2024 09:02

martin-fleck-at force-pushed the generate-index branch from 94d90bc to e7f628d Compare April 15, 2024 07:12

Add 'generate-index' function to GLSP CLI

d90c219

- Turn CLI package into ES Module so we can easily use 'globby' - Minor fix in 'check-headers' if second line does not contain year

martin-fleck-at force-pushed the generate-index branch from e7f628d to d90c219 Compare April 15, 2024 07:55

tortmayr self-requested a review April 16, 2024 09:21

tortmayr requested changes Apr 16, 2024

View reviewed changes

PR Feedback: Multiple validated root directories, Keep existing Headers

36ce743

tortmayr reviewed Apr 16, 2024

View reviewed changes

PR Feedback: Use LOGGER, remove console.log, simplify header parsing

0bc6457

tortmayr approved these changes Apr 17, 2024

View reviewed changes

tortmayr merged commit 063af47 into master Apr 17, 2024
5 checks passed

tortmayr deleted the generate-index branch April 17, 2024 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'generate-index' function to GLSP CLI #1197

Add 'generate-index' function to GLSP CLI #1197

martin-fleck-at commented Dec 15, 2023

tortmayr left a comment

martin-fleck-at commented Mar 22, 2024

tortmayr commented Mar 23, 2024

martin-fleck-at commented Apr 15, 2024

tortmayr left a comment

martin-fleck-at commented Apr 16, 2024

tortmayr left a comment

tortmayr Apr 16, 2024

martin-fleck-at Apr 16, 2024

tortmayr Apr 16, 2024

tortmayr commented Apr 16, 2024

martin-fleck-at commented Apr 16, 2024

tortmayr left a comment

Add 'generate-index' function to GLSP CLI #1197

Add 'generate-index' function to GLSP CLI #1197

Conversation

martin-fleck-at commented Dec 15, 2023

tortmayr left a comment

Choose a reason for hiding this comment

martin-fleck-at commented Mar 22, 2024

tortmayr commented Mar 23, 2024

martin-fleck-at commented Apr 15, 2024

tortmayr left a comment

Choose a reason for hiding this comment

martin-fleck-at commented Apr 16, 2024

tortmayr left a comment

Choose a reason for hiding this comment

tortmayr Apr 16, 2024

Choose a reason for hiding this comment

martin-fleck-at Apr 16, 2024

Choose a reason for hiding this comment

tortmayr Apr 16, 2024

Choose a reason for hiding this comment

tortmayr commented Apr 16, 2024

martin-fleck-at commented Apr 16, 2024

tortmayr left a comment

Choose a reason for hiding this comment