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

feat(developer): verify that packages do not contain themselves in kmc-package #13157

Open
wants to merge 2 commits into
base: feat/developer/13134-improve-messages-and-links
Choose a base branch
from

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Feb 7, 2025

@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Feb 7, 2025

User Test Results

Test specification and instructions

User tests are not required

Copy link
Contributor

@darcywong00 darcywong00 left a comment

Choose a reason for hiding this comment

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

lgtm

return true;
}

private doesContentFileEqualOutputFilename(file: KmpJsonFile.KmpJsonFileContentFile, outputFilename: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with the other methods, maybe rename to

Suggested change
private doesContentFileEqualOutputFilename(file: KmpJsonFile.KmpJsonFileContentFile, outputFilename: string): boolean {
private checkIfContentFileEqualsOutputFilename(file: KmpJsonFile.KmpJsonFileContentFile, outputFilename: string): boolean {

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually didn't use checkIf deliberately, because checkIf... also reports a message, whereas does... just checks without reporting. Probably would be better to refactor checkIf... functions to just does... in the future, but super lowpri.

Copy link
Contributor

@darcywong00 darcywong00 left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

feat(developer): prevent package from including itself (or version of itself)
2 participants