Skip to content

Multiple changes #1

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Multiple changes #1

wants to merge 3 commits into from

Conversation

gorriecoe
Copy link

Hi @adoyle-h,

Great idea for a package. I have made a couple of changes that I think will make a bit more reliable.

Added string-template for formating.   Refer to https://www.npmjs.com/package/string-template
Removed L file.  Now calls logic file directly.
Updated documentation to suite.
@adoyle-h
Copy link
Owner

Hi @gorriecoe ,

These changes are reasonable. But there is something wrong with code style. Please change it and make sure all code style checking and test cases passed in CI. I will merge it and publish at version 1.0.0.

Thanks for your contributions.

@gorriecoe
Copy link
Author

Hi @adoyle-h,

I have fixed all errors except your last test.

@adoyle-h
Copy link
Owner

test('throw error when no matched variable ', (t) => {
    const error = t.throws(() => logic('!{a}', {ha: false}));
    t.is(error instanceof Error, true);
    t.is(error.message, 'No any matched variable for "a"');
});

I expect to throw an error when no matched variable in the string template.
string-template('!{a}', {ha: false})) will return "!", and then eval("!") will cause the "Unexpected end of input" error.
I think missing variable is unstable and dangerous. It is better to throw error immediately at this line.

I suggest you copy and modify string-template codes (with its copyright notice) into the logic-string repo for full control. Or make a PR to the string-template repo.

@gorriecoe

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.

None yet

2 participants