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

chore(tools): add check flag to browser_compat.ts #4240

Merged

Conversation

timreichen
Copy link
Contributor

  • adds check flag to the browser_compat.ts tool to check missing comments in file. It will inject missing comments if the flag is not present.
    This feature is borrowed from the check_licence.ts tool.

@timreichen timreichen requested a review from kt3k as a code owner January 25, 2024 08:47
const index = source.indexOf(COPYRIGHT);
await Deno.writeTextFile(
path,
source.slice(0, index + COPYRIGHT.length) + "\n" + DECLARATION + "\n" +
Copy link
Member

Choose a reason for hiding this comment

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

The second "\n" looks redundant to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

This section makes me wonder whether we should integrate both copyright and web compatibility checks. Either way, an idea for another issue/PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second "\n" looks redundant to me.

removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section makes me wonder whether we should integrate both copyright and web compatibility checks. Either way, an idea for another issue/PR.

I agree, this looks ugly and hacky. I had a try with the ts compiler API which would work without direct string manipulation, but the ts printer doesn't retain format and whitespaces. The same problem appeared on a draft of auto-inserting warnOnDeprecatedApi() calls for deprecated functions. Is there a simple ts parser that also retains format and whitespaces?

import { walkSync } from "../fs/walk.ts";

import { walk } from "../fs/walk.ts";
import { COPYRIGHT } from "./check_licence.ts";

const ROOT = new URL("../", import.meta.url);
const SKIP = [/(test|bench|\/_|\\_|testdata|version.ts)/];
const DECLARATION = "// This module is browser compatible.";
Copy link
Member

Choose a reason for hiding this comment

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

Not directly related to this PR, but some file (data_structures/comparators.ts) seems having /** This module is browser compatible. */ comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I guess we should replace that with a single line comment in another PR.

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Great idea!

const index = source.indexOf(COPYRIGHT);
await Deno.writeTextFile(
path,
source.slice(0, index + COPYRIGHT.length) + "\n" + DECLARATION + "\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

This section makes me wonder whether we should integrate both copyright and web compatibility checks. Either way, an idea for another issue/PR.

@kt3k kt3k changed the title feat(tools): add check flag to browser_compat.ts chore(tools): add check flag to browser_compat.ts Feb 4, 2024
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@kt3k kt3k merged commit bcfec95 into denoland:main Feb 5, 2024
12 checks passed
@timreichen timreichen deleted the tools_add_check_flag_to_browser_compat branch July 2, 2024 06:58
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.

3 participants