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: warn users about missing LICENSE #628

Merged
merged 7 commits into from
Oct 13, 2021

Conversation

Andrewnt219
Copy link
Contributor

@Andrewnt219 Andrewnt219 commented Oct 8, 2021

Motivation

Fixes #603

When the code fails to automatically detect a LICENSE file, it now warns the users if they want to continue.

Test Plan

Run vsce package without a LICENSE should generate a warning message.

# No LICENSE. 
# No 'SEE LICENSE IN term.md'

PS C:\helloworld> vsce package
Executing prepublish script 'npm run vscode:prepublish'...

> helloworld@0.0.1 vscode:prepublish
> npm run compile


> helloworld@0.0.1 compile
> tsc -p ./

 WARNING LICENSE.md or LICENSE.txt not found
Do you want to continue? [Y/N]

# No LICENSE. 
# Has 'SEE LICENSE IN term.md'

PS C:\helloworld> vsce package
Executing prepublish script 'npm run vscode:prepublish'...

> helloworld@0.0.1 vscode:prepublish
> npm run compile


> helloworld@0.0.1 compile
> tsc -p ./

 WARNING  term.md not found
Do you want to continue? [Y/N]

# Has LICENSE. 
# Has 'SEE LICENSE IN term.md'

PS C:\helloworld> vsce package
Executing prepublish script 'npm run vscode:prepublish'...

> helloworld@0.0.1 vscode:prepublish
> npm run compile


> helloworld@0.0.1 compile
> tsc -p ./

 WARNING  term.md not found
Do you want to continue? [Y/N]

Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

I have read through the README, but I don't know how I can set up dev environment to reporuce the case.

Yeah, the current steps seems to be broken. I've fixed those in main.

src/package.ts Outdated Show resolved Hide resolved
src/package.ts Outdated Show resolved Hide resolved
src/package.ts Outdated Show resolved Hide resolved
@joaomoreno joaomoreno self-assigned this Oct 11, 2021
@Andrewnt219 Andrewnt219 force-pushed the 603-warn-missing-license branch from 2304ec7 to 47f8256 Compare October 12, 2021 03:49
@Andrewnt219 Andrewnt219 force-pushed the 603-warn-missing-license branch from 47f8256 to 90d6112 Compare October 12, 2021 03:52
@Andrewnt219 Andrewnt219 marked this pull request as draft October 12, 2021 03:54
@Andrewnt219 Andrewnt219 marked this pull request as ready for review October 12, 2021 03:57
@Andrewnt219
Copy link
Contributor Author

Andrewnt219 commented Oct 12, 2021

@joaomoreno I have resolved those issues you mentioned.

Just a bit uncertain about the LICENSE name. Do you want me to extract that matching logic into a private method of LicenseProcessor, and use that in both the construct and onEnd? Or just keep it that way?

@joaomoreno
Copy link
Member

Yeah I would store that into a private expectedLicenseName: string property, in the ctor, so we don't have to compute it again.

@Andrewnt219
Copy link
Contributor Author

Done. You can refer to the Test Plan for sample console output.

src/package.ts Outdated Show resolved Hide resolved
@Andrewnt219 Andrewnt219 force-pushed the 603-warn-missing-license branch from 261862a to cc205d4 Compare October 12, 2021 18:45
@Andrewnt219
Copy link
Contributor Author

Andrewnt219 commented Oct 12, 2021

Somehow, the last commit removed some brackets from the method onFile above it, which made some tests failed. I fixed it.

Co-authored-by: João Moreno <mail@joaomoreno.com>
@Andrewnt219 Andrewnt219 force-pushed the 603-warn-missing-license branch from cc205d4 to e92377b Compare October 12, 2021 18:49
@Andrewnt219
Copy link
Contributor Author

Andrewnt219 commented Oct 12, 2021

ah, it is because of the dot in LICENSE.md or LICENSE.txt. I'll revert the regex of default back to the original.

Fixed test [toVsixManifest] should automatically detect license files:
@Andrewnt219 Andrewnt219 force-pushed the 603-warn-missing-license branch from 0c886f5 to 1f867a6 Compare October 12, 2021 19:05
src/package.ts Outdated Show resolved Hide resolved
src/package.ts Outdated Show resolved Hide resolved
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

Added a few more commits on top.

src/package.ts Outdated Show resolved Hide resolved
src/package.ts Outdated Show resolved Hide resolved
@joaomoreno joaomoreno merged commit 588ad79 into microsoft:main Oct 13, 2021
@joaomoreno
Copy link
Member

Thanks!

@joaomoreno joaomoreno added this to the October 2021 milestone Oct 13, 2021
@Andrewnt219
Copy link
Contributor Author

@joaomoreno No problem. I'd also be really happy if you can label this PR as hacktoberfest-accepted 😀.

@lnicola
Copy link

lnicola commented Dec 3, 2021

I'm not sure how this is supposed to work, but if the license is already defined as a SPDX string in package.json, the (previous) code still looked for a file on disk. Is that intentional?

@joaomoreno
Copy link
Member

joaomoreno commented Dec 6, 2021

@lnicola The SPDX string only changes the licence file name to look for. It will always look for a license file.

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

Successfully merging this pull request may close these issues.

Prevent / Warn about missing LICENSE
3 participants