-
Notifications
You must be signed in to change notification settings - Fork 96
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
Add lesson 2 with simple console calc #3
Conversation
77a6a5d
to
3b4cdc9
Compare
3b4cdc9
to
e66933e
Compare
src/lesson2/engine.test.ts
Outdated
@@ -0,0 +1,39 @@ | |||
import { firstPrioritiesCalc, secondPrioritiesCalc } from "./engine"; | |||
|
|||
test("firstPrioritiesCalc: [1, * 32]", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
просто комментарий - я привык к структуре describe(blockName) / it(testCase)
- вроде это чаще с jest встречается
src/lesson2/parser.ts
Outdated
import { isNumber } from "./helpers"; | ||
import { mathOperators } from "./mathOperators"; | ||
|
||
export type ParsedLineType = Array<number | string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А как использовать интерфейс, если у меня скалярный тип? Можно пример?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Откровенно говоря, для меня конструкция Array<number | string>
более очевидна и предпочтительная vs (number | string)[]
В целом, можно сослаться и довериться твоему опыту, попробую использовать, спасибо!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
дело вкуса, я просто дал ссылку на тот стайлгайд, который внутри команды TS используется
любой вариант валиден, если он линтер проходит ))
src/lesson2/engine.ts
Outdated
mathOperatorsPriorities, | ||
} from "./mathOperators"; | ||
|
||
const { FIRST, SECOND } = mathPriorities; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
в общем случае (когда может быть больше 2 приоритетов) это был бы масссив
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Лично мне не хватило подробной истории (чтобы по коммитам можно было проследить ход разработки)
@vvscode все замечания поправил и подкинул в PR! Спасибо за проверку! |
src/lesson2/runner.test.ts
Outdated
test("runner: 2 * 32", () => { | ||
expect(runner("2 * 32")).toEqual(64); | ||
}); | ||
it("runner: 2 * 32", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case you have describe
- no need to put that prefix runner:
manually
when you run jest
in verbose mode - it renders it as sublists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good catch, now everything according with the describe/it
notation
No description provided.