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(cli): add lb4 copyright command to generate/update file headers #4994

Merged
merged 3 commits into from
Apr 2, 2020

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Mar 30, 2020

Add lb4 copyright command to update JS/TS file headers with copyright and license. Ported from strong-tools with enhancement to support lerna managed monorepo.

Todos:

  • Add tests
  • Add docs

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

Copy link
Member

@bajtos bajtos 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 👍

Since this new command is a part of the public package @loopback/cli, we need to write some documentation.

  • As a LB user dealing with copyright headers, how am I going to discover that LB4 CLI provides a tool for my situation?
  • As a LB user not using copyright headers yet, when I discover a new copyright command in CLI help, where can I learn more about this command and whether (or when) to use it in my project?
  • As a LB user interested in using this new command, how can I learn about the configuration options - how to set the copyright owner, etc.

const LICENSE = [
'This file is licensed under the <%= license %>.',
'License text available <%= ref %>',
];
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered using ES6 template literals instead of lodash templates?

function license({license, ref}) {
  return [
    `This file is licensed under the ${license}.`,
    `License text available ${ref}`,
  ];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit involved. We can defer it for future improvement.

Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

+1 on having docs on the usage. Other than that, LGTM. Thanks @raymondfeng !

Copy link
Member

@dhmlau dhmlau 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 a question about the default owner value.
Please also update https://github.com/strongloop/loopback-next/blob/master/docs/site/Command-line-interface.md to include the command as well.

Other than that, LGTM. No need further reviews from me. Thanks.

docs/site/Copyright-generator.md Show resolved Hide resolved
@raymondfeng
Copy link
Contributor Author

Updated docs/site/tables/lb4-project-commands.html

@raymondfeng raymondfeng force-pushed the cli-copyright branch 2 times, most recently from cc3cd67 to 194d1f3 Compare March 30, 2020 21:48
@raymondfeng raymondfeng requested a review from bajtos March 30, 2020 22:48
@raymondfeng raymondfeng changed the title [RFC] feat(cli): add command to generate/update file headers feat(cli): add command to generate/update file headers Mar 31, 2020
@achrinza
Copy link
Member

achrinza commented Mar 31, 2020

It's probably out-of-scope, but we could create a future improvement PR to...

  1. Leverage spdx-license-list

  2. Utilize SPDX-License-Identifier

I'm wondering if it should be ported into the LoopBack CLI. A copyright header management tool may be better-suited as a separate package that can be depended upon and called from the LoopBack CLI. This would allow users to utilize the tool a-la carte without needing the entire LoopBack CLI.

@raymondfeng raymondfeng force-pushed the cli-copyright branch 3 times, most recently from e1473e2 to 2442a09 Compare March 31, 2020 19:53
@raymondfeng
Copy link
Contributor Author

Leverage spdx-license-list
Utilize SPDX-License-Identifier

Thank you for the suggestion. I would like to integrate with the module but it will add at least 7MB to the dependency tree :-(.

I'm wondering if it should be ported into the LoopBack CLI. A copyright header management tool may be better-suited as a separate package that can be depended upon and called from the LoopBack CLI. This would allow users to utilize the tool a-la carte without needing the entire LoopBack CLI.

The utility is ported from https://www.npmjs.com/package/strong-tools and improved with lerna support. We can possibly break down cli module into a few packages in the future and introduce extensibility via @loopback/context.

@raymondfeng
Copy link
Contributor Author

@achrinza I have improved the PR to use spdx list and https://www.npmjs.com/package/inquirer-autocomplete-prompt. But it seems to be causing test issues.

@raymondfeng raymondfeng force-pushed the cli-copyright branch 3 times, most recently from 92fcce6 to 56029c5 Compare April 1, 2020 16:40
@raymondfeng raymondfeng changed the title feat(cli): add command to generate/update file headers feat(cli): add lb4 copyright command to generate/update file headers Apr 1, 2020
@raymondfeng raymondfeng added CLI developer-experience Issues affecting ease of use and overall experience of LB users labels Apr 2, 2020
@dhmlau dhmlau added this to the April 2020 milestone Apr 2, 2020
@raymondfeng raymondfeng merged commit 2032d87 into master Apr 2, 2020
@raymondfeng raymondfeng deleted the cli-copyright branch April 2, 2020 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI developer-experience Issues affecting ease of use and overall experience of LB users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants