ForestAdmin / lumber

Install Forest Admin in minutes.
https://www.forestadmin.com
MIT License
2.08k stars 106 forks source link

refactor: reduce sequelize-column-type-getter code complexity #434

Closed rap2hpoutre closed 4 years ago

rap2hpoutre commented 4 years ago

Description

See: https://github.com/ForestAdmin/lumber/pull/430 The goal is to avoid this: https://github.com/ForestAdmin/lumber/pull/430/files#diff-0ba9ef715c22f8ab287bcf3e5eb8e25bR76

Explantation here: https://github.com/ForestAdmin/lumber/pull/424/files#r422876644 (copy/paste below)

Copy/paste

I feel like we should try to avoid introducing cognitive complexity. The complexity was already there, you added one line, so it's not your responsibility. We also should not mix refactor and feature introduction.

But, I think we could prefer a refactoring task over adding eslint-disable-next-line. WDYT?

Suggestion (original description)

A small refactor could be replacing all these occurrence (they are considered as "complex" by the linter, we could consider these as duplicate):

case type.match(/^SMALLINT.*/i) || {}).input:

With:

case typeStartingWith(type, 'SMALLINT'):

Then create a function:

function typeStartingWith(type, value) {
  return type.match(new RegExp(`^${value}.*`, 'i') || {}).input;
}

I just checked (I did not tested it though), it's enough to reduce the complexity detected by sonarJS. It also removes 11 duplicates lines. Still, I'm aware this is not related to this actual issue. Feel free to suggest something else (however, I feel like we have to do something to address this eslint-disable issue).

Pull Request checklist:

SteveBunlon commented 4 years ago

I did not test manually, I let you confirm this 👍

forest-bot commented 4 years ago

:tada: This PR is included in version 3.6.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: