The Wayback Machine - https://web.archive.org/web/20200705182500/https://github.com/strongloop/loopback-next/issues/4674
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

eslint: forbid boolean function arguments #4674

Open
bajtos opened this issue Feb 18, 2020 · 3 comments
Open

eslint: forbid boolean function arguments #4674

bajtos opened this issue Feb 18, 2020 · 3 comments

Comments

@bajtos
Copy link
Member

@bajtos bajtos commented Feb 18, 2020

Function arguments of type boolean are considered as a bad practice because they make the code difficult to read and reason about.

Consider the following code:

ctx.getBinding('repositories.todo', true);

What does the true flag mean? There is no way to tell when reading the code using the function, we have to look up the API definition and/or implementation to understand.

Compare with the current implementation which is following the best practices:

ctx.getBinding('repositories.todo', {optional: true});

Now it's more clear that we want to treat the binding as optional and don't trigger an error when it's not found.

Let's improve our eslint configuration to automatically detect and reject boolean arguments. Eslint rule to use: no-inferrable-types

Further reading:

Acceptance criteria

  • A commit modifying our eslint configuration to enable the new check. This is a breaking change, see Describe incompatibilites for release notes
  • One or more commits fixing any violations of this new rule in our existing code. For functions (methods) that are part our public API, decide whether to disable this rule via a code comment, change the method signature in a breaking (semver-major) change or implement both variants (with an options arg, with a boolean arg for backwards compatibility) to avoid a breaking change.

Breaking changes must be committed in such way that they don't trigger semver-major release of packages that are not affected.

@achrinza
Copy link
Member

@achrinza achrinza commented Feb 20, 2020

Is no-inferrable-types the correct eslint rule? AFAIK it's to prevent unnecessary verbosity when declaring a variable or parameter type.

Though it's still a good rule to enable nonetheless.

@raymondfeng
Copy link
Member

@raymondfeng raymondfeng commented Feb 20, 2020

@achrinza no-inferrable-types is already enforced to avoid something like const x : string = 'abc';.

@bajtos
Copy link
Member Author

@bajtos bajtos commented Mar 6, 2020

Is no-inferrable-types the correct eslint rule? AFAIK it's to prevent unnecessary verbosity when declaring a variable or parameter type.

Hmm, that's a good question. Now that I am re-reading the documentation for no-inferrable-types, I am not sure why I thought this is the rule to use 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.