Enable `require-jsdoc` lint and add two lints related to React #6403

somebody1234 · 2023-04-24T16:05:00Z

Pull Request Description

Enables the require-jsdoc lint
Fixes all lint errors caused by enabling this lint.

Important Notes

There is no option to require JSDoc for other constructs, like top-level constants.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

…to functions

somebody1234 · 2023-04-24T16:23:26Z

oops :(

somebody1234 · 2023-04-24T16:25:29Z

umm

somebody1234 · 2023-04-24T16:26:12Z

not sure why prettier workflow failed, local run of prettier -w . doesn't change anything. it's says lib/dashboard/src/authentication/src/authentication/cognito.ts is formatted wrongly but i don't get any changes here

PabloBuchu · 2023-04-26T07:06:33Z

@somebody1234 even this PR has some weird issues on first electron open after build. Would you mind taking a look?

somebody1234 · 2023-04-26T07:14:08Z

@PabloBuchu hmm... unfortunately still can't repro :(
steps taken:

./run ide build --skip-version-check --skip-wasm-opt --wasm-profile=dev --backend-source release --backend-rele
ase nightly

~/dist/ide/enso-linux-2023.1.1-dev.AppImage -debug.dev-tools -authentication -feature-preview.new-dashboard

the first time i wasn't logged in though, rebuilt and still cannot repro. i can try a clean build?

wdanilo

Oh wow, I feel for you. Looks like a pretty tiring job with adding docs everywhere!

wdanilo · 2023-04-27T03:09:59Z

app/ide-desktop/lib/client/src/index.ts

-    /** Set Chrome options based on the app configuration. For comprehensive list of available
+    /** Sets Chrome options based on the app configuration. For comprehensive list of available


In all of our Rust codebase we have the opposite rule - instead of "Sets" we use "Set". I think we should be consistent here. Using docs in the imperative form rather than 3rd person form makes them shorter and unified with Rust codebase (which is unified with official Rust style guide).

ah - i think the convention for TS in the other files was to use the opposite, but should be easy enough to change back

wdanilo · 2023-04-27T03:10:52Z

app/ide-desktop/lib/client/src/index.ts

-        const add = (option: string, value?: string) =>
+        /** Adds the specified Chrome option. */
+        function add(option: string, value?: string) {
            chromeOptions.push(new configParser.ChromeOption(option, value))
+        }


why we need to change const lambdas int functions? This makes the code more verbose.

wdanilo · 2023-04-27T03:11:55Z

app/ide-desktop/lib/client/src/project-management.ts

+    //   to generate a new target directory name;
+    // * contain the project files directly - in this case, the archive filename will be used
+    //   to generate a new target directory name.
+    // We try to tell apart these two cases
+    // by looking at the common prefix of the paths of the files in the archive.
    // If there is any, everything is under a single directory, and we need to strip it.


these docs are strangely formatted now. Some lines are too short.

wdanilo · 2023-04-27T03:13:51Z

app/ide-desktop/eslint.config.js

+                        FunctionDeclaration: true,
+                        MethodDefinition: true,
+                        ClassDeclaration: true,
+                        ArrowFunctionExpression: true,


I think that requiring docs on arrow fn exprs is an overkill. Vast majority of them are small helper functions that do not require better docs than just a good name. In case we will find a code during review with too long such an arrow expression, we can always refactor it to a normal fn :)

wdanilo · 2023-04-27T03:14:26Z

app/ide-desktop/lib/dashboard/src/authentication/src/dashboard/components/userMenu.tsx

@@ -16,6 +16,7 @@ interface UserMenuItemProps {
    onClick?: React.MouseEventHandler<HTMLDivElement>
 }

+/** User menu item */


missing dot

wdanilo · 2023-04-27T03:15:25Z

Ofc, please remember about Q/A before merging.

…lints

somebody1234 · 2023-04-28T08:01:44Z

files with missing sections found using this fish shell command:

for f in (find . -not \( -name node_modules -prune \) \( -name '*.ts' -o -name '*.tsx' -o -name '*.js' \) ); grep "====" $f > /dev/null || echo $f; end

somebody1234 · 2023-05-01T04:07:29Z

/** and */ being on their own line should now be all removed.
the two exceptions are after @see which incorrectly triggers the jsdoc/no-multi-asterisks lint.

grep -Ir '/\*\*$\|^ \+\*/' --exclude-dir=node_modules

similarly with all lines over 100 characters long

grep -Ir '.\{101,\}' --exclude-dir=node_modules --include='*.ts' --include='*.js'

wdanilo · 2023-05-03T14:08:27Z

I was reviewing this a week ago - still no Q/A - who is handling Q/A here?

somebody1234 · 2023-05-04T08:09:01Z

@PabloBuchu @indiv0 could one of you perhaps do a QA?

mwu-tow · 2023-05-08T23:30:56Z

@somebody1234
Please update this against the current develop. I've tried building this and running but I see some issues in the node editor that are not present on develop. I suppose they are not related to your changes, but still, I'd like to be able to QA this after merge conflicts are resolved.

somebody1234 · 2023-05-09T03:34:26Z

@mwu-tow merged, but i'm not sure any of the commits merged in would fix the issue you encountered - it was only missing the last 2 commits, so:

the TS lint revert
the "fix engine version check" commit

mwu-tow · 2023-05-09T22:15:56Z

@somebody1234
Huh, to be honest, I have no idea why (it might have been just rebuilt) but after testing now all looks good!
🟢 and sorry for the confusion.

PabloBuchu · 2023-05-10T10:20:11Z

@somebody1234 please merge with develop and solve conflicts so we can merge it :)

PabloBuchu · 2023-05-11T09:19:30Z

@somebody1234 ping

somebody1234 · 2023-05-11T09:31:26Z

whoops, fixed @PabloBuchu. there are some minor changes, but they should all be changes to comments

sylwiabr · 2023-05-12T04:50:33Z

@somebody1234 what is the status here?

somebody1234 · 2023-05-12T04:53:40Z

@sylwiabr should be ready, it's just had quite a few merge conflicts

PabloBuchu · 2023-05-12T10:22:49Z

QA 🟢

somebody1234 · 2023-05-19T15:19:38Z

QA:
dashboard actions, tested using watch-dashboard.
✔️ rename project
✔️ delete project
✔️ create empty project
✔️ create project from template
✔️ delete project
✔️ create folder
✔️ create secret
✔️ delete secret
✔️ create file (upload via form) - note: errors when the file does not have an extension with "unable to upload file". it may be desirable to pass-through the error "file name must be in the format <file_name>." instead.
✔️ create file (button at top)
✔️ delete file
✔️ open help chat
✔️ filter using search bar
✔️ open user menu
✔️ change password (user menu) - onSubmit is fired twice; fixed in another PR.
✔️ sign out (user menu)
tested using ide build:
✔️ log in (email + password)
✔️ forgot password
✔️ reset password (incorrect code)
✔️ reset password (correct code)
✔️ switch backends
⚠️ oauth login works - i'm on linux so i'm unable to test. however, i got this warning while attempting to login anyway:

VM4 sandbox_bundle:2 MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 open-deep-link listeners added

testing that frontend recompile watcher works:
✔️ ide watch
testing that live reload works:
✔️ gui watch - was navigating to / on reload, losing the query string; fixed in another PR
✔️ watch-dashboard

flags, tested using ide build:
✔️ -startup.entry=grid_view, dashboard enabled (should not appear on alternate entry points)
✔️ -startup.entry=grid_view
⚠️ -startup.project, dashboard enabled - this is being fixed in another PR.
✔️ -startup.project

…t-rename * develop: Widgets, Vector as Column, Viz Fixes and Rename Columns (#6768) Implement simple variants of `parse` for the Database backend (#6731) Enable `require-jsdoc` lint and add two lints related to React (#6403) Decimal/Integer .round and .int #6654 (#6743) Set suggestion reexports when serializing the library (#6778) Fix file uploading node expression. (#6689) Using WarningsLibrary to query for warnings (#6751) Implement `cast` for Table and Column (#6711) Display Initializing project... message when initializing project (#6661) Only send suggestions updates when type changes (#6755) sbt runEngineDistribution --debug to ease debuggging (#6745) Display "modified at" column on the cloud dashboard (#6687) Meta.meta Integer . methods (#6740) Show spinner while loading directory (#6714) Add cloud dashboard to changelog (#6688) Fix list editor panics during insertion (#6540) Update bug-report.yml Remove project create form (#6710) Change full-screen visualisation shortcut to shift-space (#6663)

* develop: (30 commits) Widgets, Vector as Column, Viz Fixes and Rename Columns (#6768) Implement simple variants of `parse` for the Database backend (#6731) Enable `require-jsdoc` lint and add two lints related to React (#6403) Decimal/Integer .round and .int #6654 (#6743) Set suggestion reexports when serializing the library (#6778) Fix file uploading node expression. (#6689) Using WarningsLibrary to query for warnings (#6751) Implement `cast` for Table and Column (#6711) Display Initializing project... message when initializing project (#6661) Only send suggestions updates when type changes (#6755) sbt runEngineDistribution --debug to ease debuggging (#6745) Display "modified at" column on the cloud dashboard (#6687) Meta.meta Integer . methods (#6740) Show spinner while loading directory (#6714) Add cloud dashboard to changelog (#6688) Fix list editor panics during insertion (#6540) Update bug-report.yml Remove project create form (#6710) Change full-screen visualisation shortcut to shift-space (#6663) Revert "Show spinner when opening/creating a project (#6321)" (#6712) ...

somebody1234 added 2 commits April 25, 2023 02:02

Add require-jsdoc lint, fix eslint errors, convert arrow functions …

cc62b69

…to functions

Fix lints

2242c69

somebody1234 added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 24, 2023

somebody1234 requested review from PabloBuchu, indiv0, MichaelMauderer and wdanilo as code owners April 24, 2023 16:05

somebody1234 changed the title ~~Wip/sb/require jsdoc~~ Enable require-jsdoc lint Apr 24, 2023

Bump prettier version; run prettier

a3db9bc

wdanilo approved these changes Apr 27, 2023

View reviewed changes

somebody1234 added 3 commits April 27, 2023 14:54

Address review

20c26d9

Merge branch 'develop' into wip/sb/require-jsdoc

7273564

Fix lint errors; increase coverage of require-jsdoc lint; add more …

9a00bc0

…lints

somebody1234 changed the title ~~Enable require-jsdoc lint~~ Enable require-jsdoc lint and add two lints related to React Apr 27, 2023

Add sections

9c15ba0

somebody1234 added 3 commits April 28, 2023 19:03

Merge branch 'develop' into wip/sb/require-jsdoc

f8e5773

Get rid of newlines adjacent to /** and */

0151739

Make all non-JSX lines under 100 characters long

00874dd

somebody1234 added 2 commits May 3, 2023 10:49

Merge branch 'develop' into wip/sb/require-jsdoc

a4e3858

Fix lints

68d9862

Merge branch 'develop' into wip/sb/require-jsdoc

d577733

somebody1234 mentioned this pull request May 4, 2023

Split dashboard.tsx into smaller components #6546

Merged

5 tasks

Merge branch 'develop' into wip/sb/require-jsdoc

0f0230e

Merge branch 'develop' into wip/sb/require-jsdoc

043ddb0

mwu-tow approved these changes May 9, 2023

View reviewed changes

Merge branch 'develop' into wip/sb/require-jsdoc

4fb4bda

somebody1234 added 2 commits May 11, 2023 19:21

Merge branch 'develop' into wip/sb/require-jsdoc

bab7a2f

Fix lint errors and comment formatting

4e91bec

Merge branch 'develop' into wip/sb/require-jsdoc

1e0aa42

PabloBuchu approved these changes May 12, 2023

View reviewed changes

somebody1234 added 2 commits May 19, 2023 22:23

Merge branch 'develop' into wip/sb/require-jsdoc

de9ec38

Fix lint errors; minor refactor

938cd82

Minor fixes

cdd39f0

somebody1234 added the CI: Ready to merge This PR is eligible for automatic merge label May 19, 2023

mergify bot merged commit 658395e into develop May 19, 2023

mergify bot deleted the wip/sb/require-jsdoc branch May 19, 2023 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `require-jsdoc` lint and add two lints related to React #6403

Enable `require-jsdoc` lint and add two lints related to React #6403

somebody1234 commented Apr 24, 2023

somebody1234 commented Apr 24, 2023

somebody1234 commented Apr 24, 2023

somebody1234 commented Apr 24, 2023

PabloBuchu commented Apr 26, 2023

somebody1234 commented Apr 26, 2023

wdanilo left a comment

wdanilo Apr 27, 2023

somebody1234 Apr 27, 2023

wdanilo Apr 27, 2023

wdanilo Apr 27, 2023

wdanilo Apr 27, 2023

wdanilo Apr 27, 2023

wdanilo commented Apr 27, 2023

somebody1234 commented Apr 28, 2023 •

edited

Loading

somebody1234 commented May 1, 2023

wdanilo commented May 3, 2023

somebody1234 commented May 4, 2023

mwu-tow commented May 8, 2023

somebody1234 commented May 9, 2023 •

edited

Loading

mwu-tow commented May 9, 2023

PabloBuchu commented May 10, 2023

PabloBuchu commented May 11, 2023

somebody1234 commented May 11, 2023

sylwiabr commented May 12, 2023

somebody1234 commented May 12, 2023

PabloBuchu commented May 12, 2023

somebody1234 commented May 19, 2023 •

edited

Loading

		/** Set Chrome options based on the app configuration. For comprehensive list of available
		/** Sets Chrome options based on the app configuration. For comprehensive list of available

Enable require-jsdoc lint and add two lints related to React #6403

Enable require-jsdoc lint and add two lints related to React #6403

Conversation

somebody1234 commented Apr 24, 2023

Pull Request Description

Important Notes

Checklist

somebody1234 commented Apr 24, 2023

somebody1234 commented Apr 24, 2023

somebody1234 commented Apr 24, 2023

PabloBuchu commented Apr 26, 2023

somebody1234 commented Apr 26, 2023

wdanilo left a comment

Choose a reason for hiding this comment

wdanilo Apr 27, 2023

Choose a reason for hiding this comment

somebody1234 Apr 27, 2023

Choose a reason for hiding this comment

wdanilo Apr 27, 2023

Choose a reason for hiding this comment

wdanilo Apr 27, 2023

Choose a reason for hiding this comment

wdanilo Apr 27, 2023

Choose a reason for hiding this comment

wdanilo Apr 27, 2023

Choose a reason for hiding this comment

wdanilo commented Apr 27, 2023

somebody1234 commented Apr 28, 2023 • edited Loading

somebody1234 commented May 1, 2023

wdanilo commented May 3, 2023

somebody1234 commented May 4, 2023

mwu-tow commented May 8, 2023

somebody1234 commented May 9, 2023 • edited Loading

mwu-tow commented May 9, 2023

PabloBuchu commented May 10, 2023

PabloBuchu commented May 11, 2023

somebody1234 commented May 11, 2023

sylwiabr commented May 12, 2023

somebody1234 commented May 12, 2023

PabloBuchu commented May 12, 2023

somebody1234 commented May 19, 2023 • edited Loading

Enable `require-jsdoc` lint and add two lints related to React #6403

Enable `require-jsdoc` lint and add two lints related to React #6403

somebody1234 commented Apr 28, 2023 •

edited

Loading

somebody1234 commented May 9, 2023 •

edited

Loading

somebody1234 commented May 19, 2023 •

edited

Loading