Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 22 commits into from
May 19, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
cc62b69
Add `require-jsdoc` lint, fix eslint errors, convert arrow functions …
somebody1234 Apr 24, 2023
2242c69
Fix lints
somebody1234 Apr 24, 2023
a3db9bc
Bump prettier version; run prettier
somebody1234 Apr 25, 2023
20c26d9
Address review
somebody1234 Apr 27, 2023
7273564
Merge branch 'develop' into wip/sb/require-jsdoc
somebody1234 Apr 27, 2023
9a00bc0
Fix lint errors; increase coverage of `require-jsdoc` lint; add more …
somebody1234 Apr 27, 2023
9c15ba0
Add sections
somebody1234 Apr 28, 2023
f8e5773
Merge branch 'develop' into wip/sb/require-jsdoc
somebody1234 Apr 28, 2023
0151739
Get rid of newlines adjacent to `/**` and `*/`
somebody1234 May 1, 2023
00874dd
Make all non-JSX lines under 100 characters long
somebody1234 May 1, 2023
a4e3858
Merge branch 'develop' into wip/sb/require-jsdoc
somebody1234 May 3, 2023
68d9862
Fix lints
somebody1234 May 3, 2023
d577733
Merge branch 'develop' into wip/sb/require-jsdoc
somebody1234 May 4, 2023
0f0230e
Merge branch 'develop' into wip/sb/require-jsdoc
somebody1234 May 8, 2023
043ddb0
Merge branch 'develop' into wip/sb/require-jsdoc
somebody1234 May 9, 2023
4fb4bda
Merge branch 'develop' into wip/sb/require-jsdoc
somebody1234 May 10, 2023
bab7a2f
Merge branch 'develop' into wip/sb/require-jsdoc
somebody1234 May 11, 2023
4e91bec
Fix lint errors and comment formatting
somebody1234 May 11, 2023
1e0aa42
Merge branch 'develop' into wip/sb/require-jsdoc
somebody1234 May 12, 2023
de9ec38
Merge branch 'develop' into wip/sb/require-jsdoc
somebody1234 May 19, 2023
938cd82
Fix lint errors; minor refactor
somebody1234 May 19, 2023
cdd39f0
Minor fixes
somebody1234 May 19, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions app/ide-desktop/eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,18 @@ export default [
...tsEslint.configs['recommended-requiring-type-checking']?.rules,
...tsEslint.configs.strict?.rules,
eqeqeq: ['error', 'always', { null: 'never' }],
'require-jsdoc': [
'error',
{
require: {
FunctionDeclaration: true,
MethodDefinition: true,
ClassDeclaration: true,
ArrowFunctionExpression: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

FunctionExpression: true,
},
},
],
'sort-imports': ['error', { allowSeparatedGroups: true }],
'no-restricted-syntax': ['error', ...RESTRICTED_SYNTAXES],
'prefer-arrow-callback': 'error',
Expand Down
3 changes: 0 additions & 3 deletions app/ide-desktop/lib/client/src/authentication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,6 @@
import * as fs from 'node:fs'
import * as os from 'node:os'
import * as path from 'node:path'
import opener from 'opener'

import * as electron from 'electron'

import * as electron from 'electron'
import opener from 'opener'
Expand Down
12 changes: 8 additions & 4 deletions app/ide-desktop/lib/client/src/bin/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ interface ConfigConfig {
export class Config {
dir: string
port: number
/** Creates a server configuration. */
constructor(cfg: ConfigConfig) {
this.dir = path.resolve(cfg.dir)
this.port = cfg.port
Expand All @@ -66,12 +67,13 @@ async function findPort(port: number): Promise<number> {
// === Server ===
// ==============

/// A simple server implementation.
///
/// Initially it was based on `union`, but later we migrated to `create-servers`. Read this topic to
/// learn why: https://github.com/http-party/http-server/issues/483
/** A simple server implementation.
*
* Initially it was based on `union`, but later we migrated to `create-servers`. Read this topic to
* learn why: https://github.com/http-party/http-server/issues/483 */
export class Server {
server: unknown
/** Creates a simple HTTP server. */
constructor(public config: Config) {}

/** Server constructor. */
Expand All @@ -83,6 +85,7 @@ export class Server {
return server
}

/** Starts the server. */
run(): Promise<void> {
return new Promise((resolve, reject) => {
this.server = createServer(
Expand All @@ -103,6 +106,7 @@ export class Server {
})
}

/** Responds to an incoming request. */
process(request: http.IncomingMessage, response: http.ServerResponse) {
const requestUrl = request.url
if (requestUrl == null) {
Expand Down
8 changes: 7 additions & 1 deletion app/ide-desktop/lib/client/src/config/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ const USAGE =
`the application from a web-browser, the creation of a window can be suppressed by ` +
`entering either '-window=false' or '-no-window'.`

/** Contains information for a category of command line options and the options it is comprised of.
*/
class Section<T> {
description = ''
entries: (readonly [cmdOption: string, option: config.Option<T>])[] = []
Expand Down Expand Up @@ -201,9 +203,12 @@ function wordWrap(str: string, width: number): string[] {
// === Chrome Options ===
// ======================

/** Represents a command line option to be passed to the Chrome instance powering Electron. */
export class ChromeOption {
/** Creates a {@link ChromeOption}. */
constructor(public name: string, public value?: string) {}

/** Returns the option as it would appear on the command line. */
display(): string {
const value = this.value == null ? '' : `=${this.value}`
return `--${this.name}${value}`
Expand Down Expand Up @@ -355,7 +360,8 @@ export function parseArgs(clientArgs: string[] = fileAssociations.CLIENT_ARGUMEN
windowSize = parsedWindowSize
}

const printHelpAndExit = (exitCode?: number) => {
/** Prints the entire help text, and exits with the specified exit code. */
function printHelpAndExit(exitCode?: number) {
printHelp({
args,
groupsOrdering: [
Expand Down
4 changes: 2 additions & 2 deletions app/ide-desktop/lib/client/src/file-associations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ function getClientArguments(): string[] {
// === File Associations ===
// =========================

/* Check if the given path looks like a file that we can open. */
/** Check if the given path looks like a file that we can open. */
export function isFileOpenable(path: string): boolean {
const extension = pathModule.extname(path).toLowerCase()
return (
Expand All @@ -98,7 +98,7 @@ export function isFileOpenable(path: string): boolean {
)
}

/* On macOS when Enso-associated file is opened, the application is first started and then it
/** On macOS when an Enso-associated file is opened, the application is first started and then it
* receives the `open-file` event. However, if there is already an instance of Enso running,
* it receives the `open-file` event (and no new instance is created for us). In this case,
* we manually start a new instance of the application and pass the file path to it (using the
Expand Down
38 changes: 19 additions & 19 deletions app/ide-desktop/lib/client/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class App {
args: config.Args = config.CONFIG
isQuitting = false

/** Initializes and runs the Electron application. */
async run() {
urlAssociations.registerAssociations()
// Register file associations for macOS.
Expand Down Expand Up @@ -76,6 +77,7 @@ class App {
}
}

/** Processes the command line arguments. */
processArguments() {
// We parse only "client arguments", so we don't have to worry about the Electron-Dev vs
// Electron-Proper distinction.
Expand Down Expand Up @@ -114,14 +116,11 @@ class App {
}
}

/** 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* Chrome options refer to: https://peter.sh/experiments/chromium-command-line-switches. */
setChromeOptions(chromeOptions: configParser.ChromeOption[]) {
const addIf = (
opt: contentConfig.Option<boolean>,
chromeOptName: string,
value?: string
) => {
/** Adds the specified Chrome option when the specified command line option is enabled. */
function addIf(opt: contentConfig.Option<boolean>, chromeOptName: string, value?: string) {
if (opt.value) {
const chromeOption = new configParser.ChromeOption(chromeOptName, value)
const chromeOptionStr = chromeOption.display()
Expand All @@ -130,8 +129,10 @@ class App {
chromeOptions.push(chromeOption)
}
}
const add = (option: string, value?: string) =>
/** Adds the specified Chrome option. */
function add(option: string, value?: string) {
chromeOptions.push(new configParser.ChromeOption(option, value))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

logger.groupMeasured('Setting Chrome options', () => {
const perfOpts = this.args.groups.performance.options
addIf(perfOpts.disableGpuSandbox, 'disable-gpu-sandbox')
Expand Down Expand Up @@ -330,7 +331,8 @@ class App {
}
}

printVersion(): Promise<void> {
/** Prints the version of the frontend and the backend. */
async printVersion(): Promise<void> {
const indent = ' '.repeat(utils.INDENT_SIZE)
let maxNameLen = 0
for (const name in debug.VERSION_INFO) {
Expand All @@ -342,22 +344,20 @@ class App {
const spacing = ' '.repeat(maxNameLen - name.length)
console.log(`${indent}${label}:${spacing} ${value}`)
}

console.log('')

console.log('Backend:')
return projectManager.version(this.args).then(backend => {
if (!backend) {
console.log(`${indent}No backend available.`)
} else {
const lines = backend.split(/\r?\n/).filter(line => line.length > 0)
for (const line of lines) {
console.log(`${indent}${line}`)
}
const backend = await projectManager.version(this.args)
if (!backend) {
console.log(`${indent}No backend available.`)
} else {
const lines = backend.split(/\r?\n/).filter(line => line.length > 0)
for (const line of lines) {
console.log(`${indent}${line}`)
}
})
}
}

/** Registers keyboard shortcuts. */
registerShortcuts() {
electron.app.on('web-contents-created', (_webContentsCreatedEvent, webContents) => {
webContents.on('before-input-event', (_beforeInputEvent, input) => {
Expand Down
Loading