Skip to content

Add lc-test.js #64

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

Merged
merged 5 commits into from
Feb 17, 2022
Merged

Add lc-test.js #64

merged 5 commits into from
Feb 17, 2022

Conversation

kazk
Copy link
Member

@kazk kazk commented Feb 17, 2022

Reduce boilerplate. More helpers can be added in the future.

example/test.js Outdated
config.purity = "Let";
config.numEncoding = "Church";
const toInt = toIntWith(config);
const { counter } = compile(solution());
Copy link
Member Author

@kazk kazk Feb 17, 2022

Choose a reason for hiding this comment

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

Maybe getSolution() is better? Or solutionCode?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think getSolution() or readSolution() is better. It makes it more natural if an author needs to work with a solution text more than just compiling, using const solution = readSolution()

Copy link
Collaborator

Choose a reason for hiding this comment

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

I hadn't realised solution would clash with const solution = compile(solution()), but I do that in several bigger kata.

getSolution has my vote.

I have a proposal for configure in which toInt is differently than it is used here.

@@ -1,13 +1,9 @@
import { assert, config as chaiConfig } from "chai";
chaiConfig.truncateThreshold = 0;
import { assert, config, toIntWith, compile, solution } from "./lc-test.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does ./lc-test.js also have to be local? (I suppose so, for the same reason as ./files.js)

Copy link
Member Author

Choose a reason for hiding this comment

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

If you want, we can move import.meta.url to test.js instead:

export const getSolution = (base) => readFileSync(new URL("./solution.lc", base), {encoding: "utf8"});
// or general
export const getRelative = (base, file) => readFileSync(new URL(file, base), {encoding: "utf8"});
import { getSolution, getRelative } from "@codewars/lambda-calculus/test";

const { counter } = compile(getSolution(import.meta.url));

const { counter } = compile(getRelative(import.meta.url, "./solution.lc"));

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm no that doesn't seem very pleasant. I think its fine with just ./lc-test.js

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like ./lc-test.js.

Would not ./lc-tests.js completely obviate the need for a separate ./files.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, files.js doesn't exist anymore.

Copy link
Collaborator

@JohanWiltink JohanWiltink left a comment

Choose a reason for hiding this comment

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

Karott and I decided on some redesign; I'm implementing that now and I'll incorporate the workspace redesign as well. It's all interconnected or tests will fail.

@@ -1,13 +1,9 @@
import { assert, config as chaiConfig } from "chai";
chaiConfig.truncateThreshold = 0;
import { assert, config, toIntWith, compile, solution } from "./lc-test.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like ./lc-test.js.

Would not ./lc-tests.js completely obviate the need for a separate ./files.js?

@JohanWiltink
Copy link
Collaborator

I have tried to do everything from this PR in mine.

If I did that right, this one can be summarily rejected. I just don't know if I did.

@kazk kazk merged commit 8752f8b into main Feb 17, 2022
@kazk kazk deleted the lc-test branch February 17, 2022 21:12
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.

3 participants