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 implementation-check on startup #155

Open
JPSchellenberg opened this issue Oct 4, 2019 · 6 comments
Open

Add implementation-check on startup #155

JPSchellenberg opened this issue Oct 4, 2019 · 6 comments
Labels
[type] feature Changes that introduce a new feature (resulting in minor-version-bump)

Comments

@JPSchellenberg
Copy link
Member

In order to improve the development we have to make sure, that reported errors are not in fact a wrong implementation.
So we should have something like try/catch blocks around calls to the implementations and check if they fail.

Or is there a better way?

@JPSchellenberg JPSchellenberg added the enhancement New feature or request label Oct 4, 2019
@sr258
Copy link
Member

sr258 commented Oct 4, 2019

I'm not sure about this...

What would the catch blocks do? Just report that the error was raised in the implementation? You can already see this in the stack trace generated by a thrown error.

@JPSchellenberg
Copy link
Member Author

Yes, I can see that. But when I get a question from someone else who is trying to implement it I often don't have the stack trace. It is mostly something like Cannot read property xzy of undefined at File and Line. And then I have to look up all the source-code files and generated code to know where the error comes from. I hope this would make the implementation more 'user-friendly' as they can see that the error is within the implementation and not our source-code.

@sr258
Copy link
Member

sr258 commented Oct 4, 2019

Could you provide an example of how the catch block might look like? I'm a bit concerned that this might make debugging even harder as there is a risk that it swallows the actual error or obscures debug information (in particular the stack trace) that could be useful.

@JPSchellenberg
Copy link
Member Author

The example is the get-method of the ContentTypeCache.

If this.storage.load('...') fails it returns undefined. When using the .filter() in line 143 it throws the Error: TypeError: Cannot read property 'filter' of undefined.

With the try/catch block we could throw something like: Missing load-method on content-storage. Please make sure your implementation of the ContentStorage is correct. For more information see..

    public async get(...machineNames: string[]): Promise<ContentType[]> {
        try {
            if (!machineNames || machineNames.length === 0) {
                return this.storage.load('contentTypeCache');
            }
            return (await this.storage.load('contentTypeCache')).filter(
                (contentType: ContentType) =>
                    machineNames.some(
                        (machineName: string) =>
                            machineName === contentType.machineName
                    )
            );
        } catch (error) {
            log(error);
            throw new Error(
                'Missing load-method on content-storage. Please make sure your implementation of the ContentStorage is correct. For more information see..'
            );
        }
    }

@JPSchellenberg
Copy link
Member Author

Another idea would be to check the implementations on startup and throw errors there. Something like an implementation-validator that checks if everything works. But it is just an idea.

@sr258
Copy link
Member

sr258 commented Oct 4, 2019

I would prefer the implementation check on startup (or some test cases that you can run?). In the example above the catch block would throw the error message regardless of what goes wrong, which might be something different than just a missing load method. It could be an error in the database of the implementation, an error in our code etc. All of this is obscured by the catch statement, and I think giving a proper error message is impossible.

@JPSchellenberg JPSchellenberg changed the title Wrap all calls to implementations in try/catch-blocks with meaningful error messages. Add implementation-check on startup Oct 12, 2019
@JPSchellenberg JPSchellenberg added [type] feature Changes that introduce a new feature (resulting in minor-version-bump) and removed enhancement New feature or request labels Oct 12, 2019
@sr258 sr258 added this to the Milestone 2 milestone Dec 1, 2019
@sr258 sr258 removed this from the Milestone 2 milestone Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[type] feature Changes that introduce a new feature (resulting in minor-version-bump)
Projects
None yet
Development

No branches or pull requests

2 participants