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): use a custom repository base class #2235

Merged
merged 1 commit into from Feb 4, 2019

Conversation

Projects
None yet
3 participants
@gczobel-f5
Copy link
Contributor

gczobel-f5 commented Jan 10, 2019

Allow the user to specify a custom Repository class to inherit from.
CLI supports custom repository name

  • via an interactive prompt
  • via CLI options
  • via JSON config

Two tests modified to use the new parameter to pass
Modified tests:

  • generates a kv repository from default model
  • generates a kv repository from decorator defined model

Checklist

  • 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

@gczobel-f5 gczobel-f5 requested review from bajtos and raymondfeng as code owners Jan 10, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Jan 10, 2019

@gczobel-f5 Thank you for the patch. Please run npm run lint:fix to format the code per our settings.

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Jan 10, 2019

We usually don't merge commits from master into the feature branch. Instead, we rebase. For example:

git checkout my-branch
git pull --rebase upstream master
git push -f
@gczobel-f5

This comment has been minimized.

Copy link
Contributor Author

gczobel-f5 commented Jan 10, 2019

We usually don't merge commits from master into the feature branch. Instead, we rebase. For example:

git checkout my-branch
git pull --rebase upstream master
git push -f

Sorry, my first PR. I missed that part of the community guidelines.

});

return _.first(artifactPaths);
}

This comment has been minimized.

@raymondfeng

raymondfeng Jan 10, 2019

Member

This can be possibly simplified by using utils to map the artifact name to a file name instead of inferring the artifact name from the file names.

This comment has been minimized.

@gczobel-f5

gczobel-f5 Jan 10, 2019

Author Contributor

You mean using AST? Actually, all the CLIs use the same logic... take the first word of the filename as the artifact name...

This comment has been minimized.

@raymondfeng

raymondfeng Jan 10, 2019

Member

No. I meant to say to use a function to infer the file name from the artifact name and do the match.

This comment has been minimized.

@gczobel-f5

gczobel-f5 Jan 15, 2019

Author Contributor

Done

@bajtos
Copy link
Member

bajtos left a comment

This is a great feature 👍

IIUC the proposed implementation, it does not distinguish between repository classes that are already bound to a model (e.g. ProductRepository) and generic repository classes that are intended to be inherited from (e.g. MyCrudRepository). As a result, the CLI allows users to generate invalid project configuration, where a repository for one model (e.g. Category) is inheriting from a repository that's already bound to another model (e.g. ProductRepository).

Can we come up with a better solution? For example, we can use a different naming convention for generic repository classes (my-crud-repository.base.ts) or expect generic repositories to be defined in a different directory (e.g. repositories/base, repositories/generic, etc.).

One more scenario to consider: I expect that many base repository classes will be shared via an npm package. In the future, we should improve our CLI to discover base repository classes from the dependencies too and include them in the list of available repositories offered to the user.

Show resolved Hide resolved packages/cli/test/fixtures/repository/index.js Outdated
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Jan 11, 2019

We usually don't merge commits from master into the feature branch. Instead, we rebase.

Sorry, my first PR. I missed that part of the community guidelines.

Maybe this information is not easy to find in our guidelines? The file DEVELOPING.md does not mention the rebase process at all. The only relevant information I could find is in the section How to rebase your branch on loopback.io.

@gczobel-f5 Any suggestions on how to improve our documentation for contributors to make it easier to find the information about the rebase process? Would you mind submitting a pull request to contribute those changes yourself?

@gczobel-f5 gczobel-f5 force-pushed the gczobel-f5:fix-2149 branch from a576e5e to 52471e4 Jan 15, 2019

@gczobel-f5

This comment has been minimized.

Copy link
Contributor Author

gczobel-f5 commented Jan 15, 2019

@gczobel-f5 Any suggestions on how to improve our documentation for contributors to make it easier to find the information about the rebase process? Would you mind submitting a pull request to contribute those changes yourself?

The page at code-contrib-lb4.html is very clear about all the process from the installation up to the commit guidelines... I think that after the commit guidelines need to be mentioned the rebase process or at least a link to the page where the rebase process is explained.

@bajtos

bajtos approved these changes Jan 18, 2019

Copy link
Member

bajtos left a comment

Looks pretty good 👍

Please work with @raymondfeng to address his comments and get his approval too.

Show resolved Hide resolved packages/cli/generators/repository/index.js Outdated
Show resolved Hide resolved packages/cli/generators/repository/index.js Outdated
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Jan 24, 2019

@gczobel-f5 Any more improvements based on @bajtos's comments before we can land it?

@gczobel-f5

This comment has been minimized.

Copy link
Contributor Author

gczobel-f5 commented Jan 24, 2019

@gczobel-f5

This comment has been minimized.

Copy link
Contributor Author

gczobel-f5 commented Jan 26, 2019

@gczobel-f5 Any more improvements based on @bajtos's comments before we can land it?

@raymondfeng Done. Sorry for the delay.

@gczobel-f5 gczobel-f5 referenced this pull request Jan 31, 2019

Merged

docs(cli): use a custom repository base class #2317

1 of 7 tasks complete
@gczobel-f5

This comment has been minimized.

Copy link
Contributor Author

gczobel-f5 commented Feb 3, 2019

@raymondfeng @bajtos changes are done from 26/Jan. Any reason this is not merged?

feat(cli): use a custom repository base class
Allow the user to specify a custom Repository class to inherit from.
CLI supports custom repository name
* via an interactive prompt
* via CLI options
* via JSON config

Two tests modified to use the new parameter to pass
Modified tests:
* generates a kv repository from default model
* generates a kv repository from decorator defined model

@bajtos bajtos force-pushed the gczobel-f5:fix-2149 branch from dcced67 to a25a52b Feb 4, 2019

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Feb 4, 2019

I have rebased the changes on top of the latest master and squashed all changes into a single commit. Let's wait for CI results before landing.

@bajtos bajtos referenced this pull request Feb 4, 2019

Open

Best practice for encrypting model properties #2095

0 of 3 tasks complete

@bajtos bajtos merged commit edbbe88 into strongloop:master Feb 4, 2019

4 of 5 checks passed

coverage/coveralls Coverage decreased (-0.008%) to 91.258%
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (dhmlau) No new issues
Details
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Feb 4, 2019

Landed, thank you @gczobel-f5 for this great improvement and sorry for the delay.

@nabdelgadir nabdelgadir referenced this pull request Mar 7, 2019

Merged

docs: add repository base class #2551

2 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.