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

Refactor single argument functions #22

Open
tshemsedinov opened this issue Aug 22, 2018 · 5 comments
Open

Refactor single argument functions #22

tshemsedinov opened this issue Aug 22, 2018 · 5 comments
Assignees

Comments

@tshemsedinov
Copy link
Member

We have different code style for single argument functions in Metarhia modules.
Sometimes we use (par) => par in other places par => par. I propose to use par => par in most places except special cases described below.
So I propose to refactor core modules: common, jstp, metasync, metaschema, globalstorage, sandboxed-fs, impress, impress-cli, metatests, metacom, concolor, metalog, tickplate, do. @JuliaGerasymenko can do this when she have a free time from front-end tasks.

Exceptions:

const functionWithDescription = (
  // In single-argument functions with large description
  par // string, meaning of par argument
  // Returns: string, meaning of result value
) => {
  // implementation
};

/cc @aqrln @lundibundi @nechaido @belochub @alinkedd

@lundibundi
Copy link
Member

If you want to refactor a simple case of (val) => res to val => res then I think one eslint --fix run with a proper rule can do that.

As for the exceptions you've provided I think we need to agree on a doc style we will use and only then touch such cases. Imo it will be better if we just describe everything before the function (something like jsdoc/javadoc/cpp-doc etc) so that the text won't clutter the actual parameters/implementation of the function and will further allow to also write them via simple val => res or

(val1, val2, val3) => 
  some-complex-but-one-line-check

Currently, my concerns are:

  • We have long comments to describe the parameters, behavior (i.e https://github.com/metarhia/jstp/blob/master/lib/server.js#L60, https://github.com/metarhia/jstp/blob/master/lib/common.js#L118 etc) which is not feasible with the style in the post above as this will clutter it too much that it will be hard to distinguish comment and parameters
  • There is even less space for comments this way and this makes them stretch down even more (in order to have it neatly and 'readably' aligned)
  • Any decent editor is able to print 'comment-above-function' for you as a function doc, whatever it is. But if we use some specific style it will make that impossible
  • When you change comments with the style above you may have to reorder the parameters and or change style that cause unnecessary diff clutter
  • It makes impossible to add comments to a one-line (2-line) simple functions as you have to split it to add a comment in-between and it will no longer be a one-liner

Also, @JuliaGerasymenko please don't refactor metatests until metarhia/metatests#66 has landed.

aqrln added a commit to metarhia/eslint-config-metarhia that referenced this issue Aug 22, 2018
Remove `requireForBlockBody` option of `arrow-body-style` rule.

BREAKING CHANGE: previously, only arrow functions consisting of a single
expression were allowed to omit parentheses around the parameter list
(and required to, if there's only one parameter in the list).  After
this change, the parens are only allowed when they are required
syntactically.

Before:

```js
const f = x => x;

const g = (x, y) => x + y;

const h = (x) => {
  console.log(x);
};

const i = (x, f) => {
  f(x);
};
```

After:

```js
// Not affected
const f = x => x;

// Not affected
const g = (x, y) => x + y;

// AFFECTED
const h = x => {
  console.log(x);
};

// Not affected
const i = (x, f) => {
  f(x);
};
```

This rule is fixable via `eslint --fix`.

Refs: metarhia/Metarhia#22
aqrln added a commit to metarhia/eslint-config-metarhia that referenced this issue Aug 22, 2018
Remove `requireForBlockBody` option of `arrow-parens` rule.

BREAKING CHANGE: previously, only arrow functions consisting of a single
expression were allowed to omit parentheses around the parameter list
(and required to, if there's only one parameter in the list).  After
this change, the parens are only allowed when they are required
syntactically.

Before:

```js
const f = x => x;

const g = (x, y) => x + y;

const h = (x) => {
  console.log(x);
};

const i = (x, f) => {
  f(x);
};
```

After:

```js
// Not affected
const f = x => x;

// Not affected
const g = (x, y) => x + y;

// AFFECTED
const h = x => {
  console.log(x);
};

// Not affected
const i = (x, f) => {
  f(x);
};
```

This rule is fixable via `eslint --fix`.

Refs: metarhia/Metarhia#22
@aqrln
Copy link
Member

aqrln commented Aug 22, 2018

@JuliaGerasymenko once metarhia/eslint-config-metarhia#11 lands and the new version of ESLint config is published, the path forward is:

  1. Checkout each repo throughout the org for which you have write access. For those you don't, fork them and then checkout your fork.

  2. Even when you fork a repo, always switch to a new branch anyway.

  3. You will be modifying package-lock.json (albeit indirectly), so make sure to use the latest npm version. If npm -v prints anything less than "6.4.0", update Node.js and run npm i -g npm@latest.

  4. Update eslint-config-metarhia to ^4.0.0 if it is present in devDependencies. If it's not, it should be, so add it instead, and replace the outdated .eslintrc.yml with .eslintrc (or .eslintrc.json, which is the same) with the following content:

    {
      "extends": "metarhia"
    }

    If npm complains about missing peer dependencies, then you also have to update (or install) eslint and eslint-plugin-import.

  5. Run npm run lint -- --fix or whatever the npm script in a particular package is (it's probably lint in all of them though). If any errors remain, fixup them manually.

  6. Commit all of those changes and submit a PR.

@tshemsedinov
Copy link
Member Author

I don't like jsdoc style, it makes sources heavy-looking. I'd prefer to minimize comments at all. But if function arguments in not obvious I'd prefer to use simple // before function or comment style I suggested above. We have a special parser for this style, @lundibundi

aqrln added a commit to metarhia/eslint-config-metarhia that referenced this issue Aug 22, 2018
Remove `requireForBlockBody` option of `arrow-parens` rule.

BREAKING CHANGE: previously, only arrow functions consisting of a single
expression were allowed to omit parentheses around the parameter list
(and required to, if there's only one parameter in the list).  After
this change, the parens are only allowed when they are required
syntactically.

Before:

```js
const f = x => x;

const g = (x, y) => x + y;

const h = (x) => {
  console.log(x);
};

const i = (x, f) => {
  f(x);
};
```

After:

```js
// Not affected
const f = x => x;

// Not affected
const g = (x, y) => x + y;

// AFFECTED
const h = x => {
  console.log(x);
};

// Not affected
const i = (x, f) => {
  f(x);
};
```

This rule is fixable via `eslint --fix`.

Refs: metarhia/Metarhia#22
@tshemsedinov
Copy link
Member Author

Done

@aqrln
Copy link
Member

aqrln commented Aug 27, 2018

@tshemsedinov it's not done, though. I've just landed a PR by @belochub in JSTP (metarhia/jstp#371), but other repos haven't been refactored/eslint --fix'ed by anyone yet.

belochub added a commit to metarhia/common that referenced this issue Oct 17, 2018
belochub added a commit to metarhia/metaschema that referenced this issue Oct 17, 2018
belochub added a commit to metarhia/common that referenced this issue Oct 18, 2018
belochub added a commit to metarhia/common that referenced this issue Oct 18, 2018
@belochub belochub reopened this Oct 18, 2018
belochub added a commit to metarhia/metaschema that referenced this issue Oct 18, 2018
belochub added a commit to metarhia/metasync that referenced this issue Oct 18, 2018
belochub added a commit to metarhia/metatests that referenced this issue Oct 18, 2018
belochub added a commit to metarhia/metasync that referenced this issue Oct 18, 2018
belochub added a commit to metarhia/metatests that referenced this issue Oct 27, 2018
belochub added a commit to metarhia/metatests that referenced this issue Oct 29, 2018
belochub added a commit to metarhia/common that referenced this issue Oct 29, 2018
belochub added a commit to metarhia/common that referenced this issue Oct 29, 2018
belochub added a commit to metarhia/common that referenced this issue Oct 29, 2018
belochub added a commit to metarhia/common that referenced this issue Oct 29, 2018
belochub added a commit to metarhia/metasync that referenced this issue Oct 31, 2018
belochub added a commit to metarhia/metasync that referenced this issue Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants