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

Ensure license headers adher to Cargo manifest #58

Merged
merged 27 commits into from
Sep 19, 2024
Merged

Conversation

rzadp
Copy link
Contributor

@rzadp rzadp commented Sep 16, 2024

  • Allows us to ensure that if a license header exists, it matches the Cargo manifest license.
  • It's needed by Compare license to Cargo.toml #44
  • Introduce an option to filter by file extensions.
    • So far, for the purpose of ensuring licenses, the tool has been used with a shell glob. However, we need to use an already-existing functionality of traversing the directories, so that we can propagate the Cargo manifest license down the line.
    • With this, we can close Revamp the include parameter #45.
  • Update the rust-crate-scanner to properly parse inherited properties in Cargo.toml.

@rzadp
Copy link
Contributor Author

rzadp commented Sep 18, 2024

@paritytech/opstooling A ping for review :)

license-scanner/scanner.ts Show resolved Hide resolved
@@ -114,7 +116,7 @@ export class DB {
}

export type RustCrateScannerOutput = {
license: string | null | undefined;
license: string | null | undefined | { workspace: true };
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like a realy weird type, and I can't find how it's used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so now we return Inheritable<> from the scanner, which is {workspace: true}, but do we somehow use it?
If we wanted just to avoid crash, why not return null there?
If we wanted to resolve inherited properties, why didn't we do it in rust scanner, why expose it to ts part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great points.

I didn't want to resolve inherited properties, I created an issue and didn't want to spend time on it, because this functionality is not being used for years.

But now that I think about it again, it should be rather simply to resolve it now.
I'll take another look at it. If not, I'll return null as you suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mutantcornholio I redid it, now I just return null instead of those inheritable types without using them.

Added a new test target - a crate with inherited properties.

Copy link
Contributor

@mutantcornholio mutantcornholio left a comment

Choose a reason for hiding this comment

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

LGTM

@rzadp rzadp merged commit 27d9ec6 into master Sep 19, 2024
3 checks passed
@rzadp rzadp deleted the rzadp/scan-files branch September 19, 2024 12:53
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.

Revamp the include parameter
2 participants