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

Implement cond #15

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Implement cond #15

wants to merge 9 commits into from

Conversation

leonid-shutov
Copy link

@leonid-shutov leonid-shutov commented Dec 15, 2024

#9

  • tests and linter show no problems (npm t)
  • tests are added/updated for bug fixes and new features
  • code is properly formatted (npm run fix)
  • description of changes is added in CHANGELOG.md
  • update .d.ts typings

@leonid-shutov leonid-shutov changed the title implement cond Implement cond Dec 15, 2024
@leonid-shutov leonid-shutov force-pushed the main branch 2 times, most recently from 73e1160 to f768415 Compare December 20, 2024 19:04
Comment on lines 75 to 106
let header = '(';

for (const identifier of this.identifiers) {
if (header.length > 1) {
header += ', ';
}
header += identifier;
}

header += ')';

return `${header} => ${this.toExpression()}`;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid creating an extra array as Murych suggested

} else if (operand instanceof OperationExpression) {
for (const name of operand.identifiers.values()) {
if (operand.identifiers !== undefined) {
for (const name of operand.identifiers) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think information expert works well here

Copy link
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Compact empty lines and one-operator blocks
  • Move oll fields to constructor

Comment on lines 73 to 106
let header = '(';
for (const identifier of this.identifiers) {
if (header.length > 1) header += ', ';
header += identifier;
}
header += ')';
return `${header} => ${this.toExpression()}`;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understood moving from Array.from to for..of solution, please explain.

Suggested change
let header = '(';
for (const identifier of this.identifiers) {
if (header.length > 1) header += ', ';
header += identifier;
}
header += ')';
return `${header} => ${this.toExpression()}`;
}
let header = '';
for (const identifier of this.identifiers) {
if (header) header += ', ';
header += identifier;
}
return `(${header}) => ${this.toExpression()}`;
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Murych suggested to avoid creating an extra array. Just not sure if concatenating strings is as fast as Array.join

Comment on lines 105 to 142
// eslint-disable-next-line consistent-return
interpret(context) {
for (const { condition, consequents } of this.clauses) {
if (condition.interpret(context) !== false) {
return consequents.reduce(
(_, consequent) => consequent.interpret(context),
undefined,
);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I do not like disabled eslint rules, just make return consistent.
  • !== false is not ok, see fix
  • It is not ok to start reducing from undefined in most cases, it is also strange to use reduce with ignored accumulator arg
Suggested change
// eslint-disable-next-line consistent-return
interpret(context) {
for (const { condition, consequents } of this.clauses) {
if (condition.interpret(context) !== false) {
return consequents.reduce(
(_, consequent) => consequent.interpret(context),
undefined,
);
}
}
}
interpret(context) {
for (const { condition, consequents } of this.clauses) {
if (condition.interpret(context)) {
return consequents.reduce(
(_, consequent) => consequent.interpret(context),
undefined,
);
}
}
}

Copy link
Author

@leonid-shutov leonid-shutov Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do if(condition.interpret(context)) because in LISP condition is false only if it evaluates to nil while in JavaScript it can be any falsy value like 0. E.g. (cond (0 x)) would be a true condition.

This is why I did !== false (nil is interpreted to false in our current solution). I've made this explicit in the latest commit.

Comment on lines 120 to 124
const consequentExpressions = consequents.map((consequent, i) =>
i === consequents.length - 1
? `return ${consequent.toExpression()};`
: `${consequent.toExpression()};`,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const consequentExpressions = consequents.map((consequent, i) =>
i === consequents.length - 1
? `return ${consequent.toExpression()};`
: `${consequent.toExpression()};`,
);
const consequentExpressions = consequents.map((consequent, i) => {
const ex = consequent.toExpression();
return i === consequents.length - 1 ? `return ${ex};` : `${ex};`;
});

lib/parser.js Outdated
Comment on lines 43 to 44
const head = tokens[0];
const tail = tokens.splice(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have head and tail utilities

@tshemsedinov
Copy link
Member

We need to see you on the call one time

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

Successfully merging this pull request may close these issues.

2 participants