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

Std extension exec tests #456

Merged
merged 34 commits into from
Nov 6, 2023
Merged

Std extension exec tests #456

merged 34 commits into from
Nov 6, 2023

Conversation

f3l1x98
Copy link
Contributor

@f3l1x98 f3l1x98 commented Oct 9, 2023

Closes #416
Note: This PR also includes #462 due to required functionality!

This PR adds tests to all the block-executors of the std extension (and also the file-util.ts).

@f3l1x98 f3l1x98 mentioned this pull request Aug 6, 2023
4 tasks
@f3l1x98
Copy link
Contributor Author

f3l1x98 commented Oct 16, 2023

I have found a few parts where I either already adjusted parts or think need adjustments. These are the ones I have thus far:

Adjusted

  1. File picker file not found.
    I have added handling in case the file was not found, because otherwise the assert(file instance of BinaryFile) would have triggered a vague error:
    const file = fileSystem.getFile(path);
    if (file == null) {
    return R.err({
    message: `File '${path}' not found`,
    diagnostic: { node: context.getCurrentNode() },
    });
    }
  2. Gtfs interpreter interpret error.
    I have added a try catch to return a better error message in case the gtfs decode fails:
    let feedMessage;
    try {
    feedMessage = GtfsRealtimeBindings.transit_realtime.FeedMessage.decode(
    new Uint8Array(inputFile.content),
    );
    } catch (e) {
    return Promise.resolve(
    R.err({
    message: `Failed to decode gtfs file: ${this.getErrorMessage(e)}`,
    diagnostic: { node: context.getCurrentNode() },
    }),
    );
    }

Require adjustments

  1. File utils method naming.
    I am not 100% sure, but I think the parameter is named wrong and should be contentType: string.
    Alternatively the name of the method should be inferMimeTypeFromFileExtensionString if I misunderstood the method itself:
    export function inferMimeTypeFromContentTypeString(
    fileExtension: string | undefined,
    ): MimeType | undefined {
    if (fileExtension !== undefined) {
    const inferredMimeType = mime.lookup(fileExtension) as MimeType;
    if (Object.values(MimeType).includes(inferredMimeType)) {
    return inferredMimeType;
    }
    }
    return undefined;
    }

@f3l1x98 f3l1x98 changed the title Draft: Std extension exec tests Std extension exec tests Oct 16, 2023
@f3l1x98 f3l1x98 requested a review from georg-schwarz October 16, 2023 10:30
Copy link
Member

@georg-schwarz georg-schwarz left a comment

Choose a reason for hiding this comment

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

Nice!

@georg-schwarz
Copy link
Member

  1. File utils method naming.
    I am not 100% sure, but I think the parameter is named wrong and should be contentType: string.
    Alternatively the name of the method should be inferMimeTypeFromFileExtensionString if I misunderstood the method itself:
    export function inferMimeTypeFromContentTypeString(
    fileExtension: string | undefined,
    ): MimeType | undefined {
    if (fileExtension !== undefined) {
    const inferredMimeType = mime.lookup(fileExtension) as MimeType;
    if (Object.values(MimeType).includes(inferredMimeType)) {
    return inferredMimeType;
    }
    }
    return undefined;
    }

From what I saw the method infers the mime type from the filename ending (e.g., .txt, .pdf). So the method name is misleading, good catch! Should be inferMimeTypeFromFileExtension instead.

@f3l1x98 f3l1x98 requested a review from georg-schwarz November 6, 2023 10:39
@f3l1x98 f3l1x98 merged commit 282fcf0 into main Nov 6, 2023
2 checks passed
@f3l1x98 f3l1x98 deleted the feature/std-extension-exec-tests branch November 6, 2023 12:44
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] std extension exec tests
2 participants