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

Refactor CLI code for easier reuse #3788

Merged
merged 6 commits into from
Sep 24, 2019
Merged

Refactor CLI code for easier reuse #3788

merged 6 commits into from
Sep 24, 2019

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Sep 23, 2019

While working on #3688 (lb4 import-lb3-model), I discovered many obstacles preventing me from reusing existing code in my new generator.

This pull request contains a series of commits to make various bits in our CLI code base easier to use in new ways.

It may be best to review the changes commit-by-commit.

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 👈

@bajtos bajtos added this to the Sept 2019 milestone milestone Sep 23, 2019
@bajtos bajtos requested review from raymondfeng, agnes512 and a team September 23, 2019 11:23
@bajtos bajtos self-assigned this Sep 23, 2019
@bajtos bajtos changed the title Refactor CLI code for better code reuse Refactor CLI code for easier reuse Sep 23, 2019
@bajtos bajtos force-pushed the refactor/cli-for-import branch from 79bd274 to 2216405 Compare September 23, 2019 11:39
Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 the naming part is pretty reasonable, makes the code more readable.
I am good with the PR once the CI passes

@@ -103,6 +104,32 @@ exports.validateClassName = function(name) {
return util.format('Class name is invalid: %s', name);
};

exports.remindAboutNamingRules = function(name, log) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is checkNamingConvention a better name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in my mind. I understand checkNamingConvention to report an error when the name does not match certain pattern(s). This function just prints a warning to explain the user what will happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe reportNamingIssues or warnInvalidName?

Copy link
Contributor

Choose a reason for hiding this comment

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

warnInputNaming?

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'll go with logNamingIssues for consistency with logClassCreation.

}
};

exports.printClassFileName = function(type, typePlural, name, log) {
Copy link
Contributor

Choose a reason for hiding this comment

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

printClassFileName => logClassGeneration?

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'll go with logClassCreation to match the message printed. I find generation ambiguous - is it describing the process of generating something new, or a generation like a group of similarly-aged organisms?

@bajtos bajtos force-pushed the refactor/cli-for-import branch 3 times, most recently from 7d7fdac to 0c1ab24 Compare September 23, 2019 15:10
Copy link
Contributor

@nabdelgadir nabdelgadir left a comment

Choose a reason for hiding this comment

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

👏

Introduce two new helpers to be used in generators not inheriting form
`ArtifactGenerator`:

- `_isGenerationSuccessful` to detect whether all target files were
  successfully created or modified.

- `_updateIndexFile` to add a single entry to a given `index.ts` file

Signed-off-by: Miroslav Bajtoš <[email protected]>
@bajtos bajtos force-pushed the refactor/cli-for-import branch from 0c1ab24 to e524742 Compare September 24, 2019 06:50
@bajtos bajtos merged commit 387411c into master Sep 24, 2019
@bajtos bajtos deleted the refactor/cli-for-import branch September 24, 2019 07:08
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.

4 participants