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

Allow specification of custom detectors + readme update #1

Closed
wants to merge 8 commits into from

Conversation

FredrikSchaefer
Copy link
Owner

@FredrikSchaefer FredrikSchaefer commented Jul 14, 2023

Resolves #340

Copy link

@BenSower BenSower left a comment

Choose a reason for hiding this comment

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

lgtm, please see my comment and potentially also add some tests for this new functionality

core.js Outdated
@@ -59,9 +59,23 @@ function _check(buffer, headers, options) {
return true;
}

export async function fileTypeFromTokenizer(tokenizer) {
async function runCustomDetectors(tokenizer, fileType, detectors) {
if (!detectors || detectors.length === 0) {

Choose a reason for hiding this comment

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

Code-Style: I would invert the condition to simplify this block and make it even cleaner to read:

if (detectors && detectors.length) {
        for (const detector of detectors) {
		fileType = detector(tokenizer, fileType);
	}
}
return fileType;

Nit: It might be noteworthy, that right now, it could happen that if there are (for whatever reason) multiple detectors that either claim to have detected the file-type they were built for or are not properly looping through the previous fileType variable, the last detector in the array will overwrite the previous ones. I don't think that this is likely, but intuitively (and potentially also to improve performance in case you have a lot of detectors), I would expect the detection to stop after the first positive result.

@FredrikSchaefer
Copy link
Owner Author

Closed in favor of this PR, which is the real thing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants