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

add wait command to CliCore and TemplateProcessor #19

Closed
wants to merge 2 commits into from
Closed

Conversation

zhirafovod
Copy link
Member

Description

Adds wait command and parameters for init.

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change
  • Refactor
  • Documentation
  • Other (please describe)

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests
  • Functionality is documented
  • All code style checks pass
  • New code contribution is covered by automated tests
  • All new and existing tests pass

@zhirafovod zhirafovod requested a review from a team as a code owner October 30, 2023 21:30
README.md Show resolved Hide resolved
src/TemplateProcessor.ts Outdated Show resolved Hide resolved
src/TemplateProcessor.ts Outdated Show resolved Hide resolved
src/TemplateProcessor.ts Outdated Show resolved Hide resolved
try {
conditionExpression = jsonata(waitCondition);
} catch (e) {
this.logger.error(`invalid wait condition expression: ${waitCondition}, ${e}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this is the right approach. I try to wrap functions in safe() so that I can simply throw error and rely on the framework to catch it and pack it into an {error:{...}} object

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried a few different approaches. I like with withErrorHandling to handling calling to an external function, where we don't know the context. In this case, I am handling errors from my own code execution, and I think we can do better with a custom error messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

but your custom error message will be used by withErrorHandling. All you need to do is throw an error:

private withErrorHandling(fn) {
        return (...args) => {
            try {
                const result = fn(...args);
                if (result instanceof Promise) {
                    return result.catch(error => {
                        this.logger.error(error.toString());
                        return {
                            "error": {
                                message: error.message,
                                name: error.name,
                                stack: error.stack,
                            }
                        };
                    });
                }
                return result;
            } catch (error) {
                this.logger.error(error.toString());
                return {
                    "error": {
                        message: error.message,
                        name: error.name,
                        stack: error.stack,
                    }
                };
            }
        };
    };

Copy link
Contributor

Choose a reason for hiding this comment

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

^^^ see how withErrorHandling preserves these properties. So why don't you just catch(e) and set it's message, and rethrow it?

geoffhendrey
geoffhendrey previously approved these changes Nov 1, 2023
try {
conditionExpression = jsonata(waitCondition);
} catch (e) {
this.logger.error(`invalid wait condition expression: ${waitCondition}, ${e}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

but your custom error message will be used by withErrorHandling. All you need to do is throw an error:

private withErrorHandling(fn) {
        return (...args) => {
            try {
                const result = fn(...args);
                if (result instanceof Promise) {
                    return result.catch(error => {
                        this.logger.error(error.toString());
                        return {
                            "error": {
                                message: error.message,
                                name: error.name,
                                stack: error.stack,
                            }
                        };
                    });
                }
                return result;
            } catch (error) {
                this.logger.error(error.toString());
                return {
                    "error": {
                        message: error.message,
                        name: error.name,
                        stack: error.stack,
                    }
                };
            }
        };
    };

try {
conditionExpression = jsonata(waitCondition);
} catch (e) {
this.logger.error(`invalid wait condition expression: ${waitCondition}, ${e}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

^^^ see how withErrorHandling preserves these properties. So why don't you just catch(e) and set it's message, and rethrow it?

src/CliCore.ts Outdated Show resolved Hide resolved
geoffhendrey
geoffhendrey previously approved these changes Nov 3, 2023
@@ -1111,6 +1129,72 @@ export default class TemplateProcessor {
return null;
}


async waitCondition(waitCondition, timeout = 10000) {
Copy link
Contributor

Choose a reason for hiding this comment

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

waitCondition and timeout arguments should have types. Return type should be explicit. Whenever we add a new method we should try to use TS best practices.


beforeEach(() => {
// Save the global variable before the test
originalDefaultFunctions = TemplateProcessor.DEFAULT_FUNCTIONS;
Copy link
Contributor

Choose a reason for hiding this comment

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

originalDefaultFunctions = [...TemplateProcessor.DEFAULT_FUNCTIONS]
If you don't do this you are still allowing the content of DEFAULT_FUNCTIONS array to be destroyed

rxLog: [ {"default": 42} ]
stop$: ($count(rxLog)=5?$clearInterval(interval$):'still going')
stop$: ($count(rxLog)=5?'done':'still going')
`;
const template = yaml.load(templateYaml);
const tp = new TemplateProcessor(template, {"subscribe": ()=>{}, "publish":()=>{}});
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need to replace subscribe and publish with empty functions anymore

@zhirafovod zhirafovod closed this Dec 13, 2023
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