StatsHouse UI: widget url parse #1517
Conversation
vauweb
force-pushed
the
vauweb/update/witget-url-parse
branch
from
0ac8ca8
to
ca84173
Compare
vauweb
force-pushed
the
vauweb/update/witget-url-parse
branch
from
ca84173
to
f8ff5fe
Compare
| expect(urlDecode(toTreeObj(arrToObj(urlEncode(params))), params)).toEqual(params); | ||
| }); | ||
| test('urlEncode => urlDecode save', () => { | ||
| expect(urlDecode(toTreeObj(arrToObj(urlEncode(params2, params))), params)).toEqual(params2); | ||
| // expect(urlDecode(toTreeObj(arrToObj(urlEncode(params2, params))), params)).toEqual(params2); |
There was a problem hiding this comment.
закомменченный код, он нужен в мастере?
There was a problem hiding this comment.
код нужен, но не как закомменченый ) правил тест выключал строку забыл вернуть, спасибо исправлю )
| import { dequal } from 'dequal/lite'; | ||
| import { metricFilterEncode } from './metricFilterEncode'; | ||
|
|
||
| export function metricEncode(plot: PlotParams, defaultPlot: PlotParams = getNewMetric()): [string, string][] { |
There was a problem hiding this comment.
столько if'ов в функции, что очень тяжело ее разобрать, думаю switch/case в данном случае был бы более читаемым
There was a problem hiding this comment.
переписать это на switch? жду ПР с предложением )
| paramArr.push([prefix + GET_PARAMS.metricFilter, keyTag + filterNotInSep + valueTag]) | ||
| ) | ||
| ); | ||
| return paramArr; |
There was a problem hiding this comment.
тут и выше в функции отступы строк было бы уместнее добавить для лучшей читаемости, как вариант, это правило в будущий линтер внести
There was a problem hiding this comment.
найдешь линтер с автофиксом или настроишь претир тогда ок, докапываться до пробелов и переносов не вижу смысла
читабельность это субъективное тебе удобнее так другому не так
| const type = | ||
| toPlotType(plotSearchParams?.[GET_PARAMS.metricType]?.[treeParamsObjectValueSymbol]?.[0]) ?? | ||
| defaultParams.plots[key]?.type; | ||
| let p: PlotParams | undefined; |
There was a problem hiding this comment.
неинтуитивный нейминг переменной, предлагаю одной буквой переменный не называть а писать как минимум слово, наиболее подходящее
There was a problem hiding this comment.
нейминг локальных временных переменных в циклах и других подобных лямда функциях которые используются сотнями, у меня плохая фантазия... при всем уважении я разделяю такой подход нормальных имен и готов его защищать... но когда нужна временная переменная которая используется в 5 строчечной лямбде... тут скорее надо придумать как написать так чтобы ненужна была временная переменная
|
|
||
| describe('urlStore widgetsParamsEncode', () => { | ||
| test('urlEncodePlots', () => { | ||
| const params2: QueryParams = { |
There was a problem hiding this comment.
params2 - некорректный нейминг
There was a problem hiding this comment.
в тесте? некоректный нейминг для параметра который params и имеет отличное от простого param значение?
давай обсудим как назвать переменную которая как params такая же но другая
а какое имя будет корректно?
No description provided.