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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): change model file naming convention #2989

Merged
merged 1 commit into from Jun 8, 2019

Conversation

Projects
None yet
6 participants
@agnes512
Copy link
Contributor

commented May 29, 2019

connect to #2644

  • Change the naming convention of Class so that the class name inputs: model1, Model1, model_1, Model___1 (mode-1 is an invalid input) will have the same model name: Model1 and the same file name model1.model.ts.
  • Add prompt warning msg that the input will get modified when it doesn't fully follow the naming convention.
  • Add prompt msg that shows which class/file names will get generated in advance.
  • The prompt message now shows the created model name instead of the input.
  • Unify naming functions as toFileName toVarName instead of kebabCase etc. Easier for maintenance.

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 馃憟

@agnes512

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Current issue:

as described in the #2644. And this is because the generator uses different conventions to name files(kebabCase) and models(pascalCase). Things go wrong when it tries to extract a list of model names by parsing file names.

Possible solutions:

A: read the file to get the model name
B: change the model file naming convention

Cases:

Input Model Name Model File Name Repo File Name
current m1 M1 m-1.model.ts m-1.repository.ts
m_1 M_1 m-1.model.ts m-1.repository.ts
solution B m1 M1 M1.model.ts m-1.repository.ts
m_1 M_1 M_1.model.ts m-1.repository.ts
M_1 M_1 M_1.model.ts m-1.repository.ts
MX_1X Mx_1X Mx_1X.model.ts mx-1-x.repository.ts

Concerns

  • If we only change the naming convention of models, there might have conflicts when generating repositories(i.e m1 and m_1 will have different model names but the same repository name). Might want to change the convention of naming repositories as well
  • Might want to change the file names to lower case
@jannyHou

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

A summary of our discussion yesterday:

Given the model name provided when creating the model by lb4 model, the class name and file name are determined as:

  • class name: the pascal case of name
  • file name: the kebab case of name

When loading the models as artifacts, it's much easier to get the file name instead of parsing the class name, but since name is not saved anywhere, it's not possible to restore the class name from the file name.

So we are thinking of generating the file name from class name instead of the name input from the prompt:

  • class name: the pascal case of name
  • file name: the kebab case of class name, then restore the class name by getting the pascal case of file name.

@strongloop/loopback-maintainers Any thought?

@raymondfeng

This comment has been minimized.

Copy link
Member

commented May 29, 2019

IMO, the simplest solution is to keep m1 as-is in file names, m1.model.ts instead of m-1.model.ts.

I also hate the fact that lb4 app m1 creates a directory as m-1.

@agnes512

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

IMO, the simplest solution is to keep m1 as-is in file names, m1.model.ts instead of m-1.model.ts.

I also hate the fact that lb4 app m1 creates a directory as m-1.

I think use the input as the its file name is doable. But it might run into some tricky issues like, the generator will take m___1 as a valid input. Then the file name will be m___1.model.ts.
I am thinking to use the same convention to name the files (solution B.1). Then change the first letter to lower case:

Input Model Name Model File Name
current m1 M1 m-1.model.ts
m_1 M_1 m-1.model.ts
solution B.1 m1 M1 m1.model.ts
M_1NN M_1Nn m_1Nn.model.ts

As for directory naming, the rest of file naming are using the kababCase as well. We can change it too.

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

The proposal for solution B.1 looks good to me. Thanks.

@agnes512 agnes512 force-pushed the modelNaming branch from aa1945b to b5f2905 Jun 3, 2019

@agnes512 agnes512 closed this Jun 4, 2019

@agnes512 agnes512 force-pushed the modelNaming branch from b5f2905 to c99a65f Jun 4, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@agnes512 Why is this PR closed? If you want to add more commits or push new commits, please keep it open as github will automatically picks up changes from the PR branch.

@agnes512 agnes512 reopened this Jun 4, 2019

@agnes512

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@agnes512 Why is this PR closed? If you want to add more commits or push new commits, please keep it open as github will automatically picks up changes from the PR branch.

That was an accident

@agnes512 agnes512 force-pushed the modelNaming branch from 456dd02 to 316b770 Jun 4, 2019

@agnes512 agnes512 marked this pull request as ready for review Jun 4, 2019

@agnes512 agnes512 requested review from bajtos and raymondfeng as code owners Jun 4, 2019

exports.pascalCase = pascalCase;
exports.camelCase = camelCase;
exports.uTilCamelCase = uTilCamelCase;

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jun 4, 2019

Contributor

Why is it uTil instead of util?

This comment has been minimized.

Copy link
@agnes512

agnes512 Jun 5, 2019

Author Contributor

fixed

@agnes512 agnes512 force-pushed the modelNaming branch from 97fbce7 to 94e9505 Jun 5, 2019

if (debug.enabled) {
debug(`Artifact output filename set to: ${this.artifactInfo.outFile}`);
}

this.artifactInfo.modelVariableName = utils.camelCase(
this.artifactInfo.modelVariableName = utils.utilCamelCase(

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jun 5, 2019

Member

Why do we rename it to utilCamelCase?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jun 5, 2019

Member

I think it should be named toVarName to be consistent with:

  • toClassName
  • toFileName

This comment has been minimized.

Copy link
@agnes512

agnes512 Jun 5, 2019

Author Contributor

true 馃憤

const pluralize = require('pluralize');
const urlSlug = require('url-slug');
const validate = require('validate-npm-package-name');
const Conflicter = require('yeoman-generator/lib/util/conflicter');
const connectors = require('../generators/datasource/connectors.json');
const stringifyObject = require('stringify-object');
const camelCase = require('camelcase');

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jun 5, 2019

Member

It's very confusing to have two camelCase? What's the difference between camelCase and change-case.camelCase?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jun 5, 2019

Member

I believe that lodash already supports camelCase.

This comment has been minimized.

Copy link
@agnes512

agnes512 Jun 5, 2019

Author Contributor

I tested it yesterday, change-case.camelCase keeps the underscores while camelCase doesn't. But we still use change-case.camelCase in this.artifactInfo.modelVariableName, so I kept it and renamed it.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jun 5, 2019

Member

I tested it yesterday, change-case.camelCase keeps the underscores while camelCase doesn't.

I think it's the other way around:

  • change-case.camelCase does not use _
  • camelCase uses _

This comment has been minimized.

Copy link
@agnes512

agnes512 Jun 5, 2019

Author Contributor

Just tested it out. I think we can get rid of the new imported camelCase since _.camelCase does the same thing.

? Model class name: to1doList_todo
NAME: to1doList_todo
change-case.camel: to1doListTodo
New imported camel: to1DoListTodo
Lodash camle :to1DoListTodo
? Model class name: to_1doList_todo
NAME: to_1doList_todo
change-case.camel: to_1doListTodo
New imported camel: to1DoListTodo
Lodash camle :to1DoListTodo

But there still a slight difference between change-case.camel and _.camel. I think we should use _.camel as we use pascal case to convert it to class name.

This comment has been minimized.

Copy link
@raymondfeng

@agnes512 agnes512 force-pushed the modelNaming branch from dfb69b7 to adfe6bb Jun 5, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@agnes512 Please fix the CI failure.

@jannyHou
Copy link
Contributor

left a comment

Good progress! Can you add some test cases to verify your fix?

@bajtos

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

My two cents, hopefully it's not too late: personally, I prefer a model name like Model2 to be stored in a file called model2.ts (not model-2.ts). Having said that, if this would be too difficult to implement, then I can live with model-2.ts too. IMO, Model2 is not a very good name, people should not really use it in production code.

What I see as most important: the conventions should be clearly described to our users. Preferably, the CLI tools should tell the user in advance about the file name it's going to create, so that they can cancel the operation and pick a better model name early on.

For example (I don't have a working LB4 CLI, sorry that the prompts don't match the actual output):

? Enter model name: Model2
Will create class Model2 in src/models/model-2.ts
? What is the base class? Entity
Using base class Entity from @loopback/repository
? Allow additional properties
(etc.)
@agnes512

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I agree that the file name for class nameModel1 to be model1.ts. But @raymondfeng suggested that it's better to apply the same convention for file naming as we might need to update lots of files.
And I think it's a good point that the CLI tools should prompt messages about the file names, class names etc. Let me try to add it to the generators馃樃

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I agree that the file name for class nameModel1 to be model1.ts. But @raymondfeng suggested that it's better to apply the same convention for file naming as we might need to update lots of files.

I'm with @bajtos that model1.ts is better than model-1.ts. My suggestion actually should make it easier to implement the convention as you just have to change utils.toFileName method now. @agnes512 Why do you think my proposal conflicts with model1.ts convention?

@agnes512

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I was using camelCase instead of kebabCase for the file names before. Then you asked me to change back to the kebab (which would create model-1.ts):. But it's fine. Should be easy to change to any cases now since we are using toFileName.

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I was using camelCase instead of kebabCase for the file names before. Then you asked me to change back to the kebab (which would create model-1.ts):. But it's fine. Should be easy to change to any cases now since we are using toFileName.

To be precise, I think @bajtos and myself are proposing a slight variant of kebab case.

  • Model1 -> model1
  • 'myModel->my-model`
  • 'your_model->your-model`
@agnes512

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I see. My bad. I don't know which library has such kebabCase. I will finish what we discussed first. Thanks for the suggestions.

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I don't know which library has such kebabCase.

Let's do this:

let fileName = _.kebabCase(str);
fileName = fileName.replace(/\-(\d+)/g, '$1');

@agnes512 agnes512 force-pushed the modelNaming branch from 3a090f1 to 9c1b0ca Jun 6, 2019

@agnes512 agnes512 force-pushed the modelNaming branch 4 times, most recently from e816d2c to ec10753 Jun 6, 2019

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

What I see as most important: the conventions should be clearly described to our users. Preferably, the CLI tools should tell the user in advance about the file name it's going to create, so that they can cancel the operation and pick a better model name early on.

+1 in capturing the information. Since it's going to affect all the CLI (I think), perhaps we can have a "naming convention" section in the CLI main page? https://loopback.io/doc/en/lb4/Command-line-interface.html. Just a thought.

)}`,
);
this.log();

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jun 6, 2019

Member

I'm seeing the prompts for a few generators. Let's refactor it into ArtifactGenerator.

this.artifactInfo.name
}`,
);
}

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jun 6, 2019

Member

To be refactored into ArtifactGenerator .

@agnes512 agnes512 force-pushed the modelNaming branch from ec10753 to 33ad019 Jun 7, 2019

@bajtos

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Since it's going to affect all the CLI (I think), perhaps we can have a "naming convention" section in the CLI main page? https://loopback.io/doc/en/lb4/Command-line-interface.html.

+1

In my experience, most people don't read docs, therefore I consider it more important to surface the naming convention in CLI messages.

let fileName = _.kebabCase(str);
fileName = fileName.replace(/\-(\d+)/g, '$1');

Edge case to consider: Car4Share - do we want car4-share.ts? I think car-4-share.ts makes more sense.

Proposed regex (notice extra $ character):

fileName = fileName.replace(/\-(\d+)$/g, '$1');

Either way, please add a test case to verify the output of toFileName('Car4Share').

@agnes512

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Either way, please add a test case to verify the output of toFileName('Car4Share').

馃憣 I will modify the test case ModelNameWithNum1 to Model1NameWithNum1

@agnes512 agnes512 force-pushed the modelNaming branch 4 times, most recently from 8c34225 to 2640a24 Jun 7, 2019

@agnes512 agnes512 force-pushed the modelNaming branch 3 times, most recently from 4578de5 to dd34feb Jun 7, 2019

@nabdelgadir
Copy link
Contributor

left a comment

馃憦

@agnes512 agnes512 force-pushed the modelNaming branch from dd34feb to a55caf6 Jun 7, 2019

fix(cli): change class/file naming convention. Add prompt msg
- change class/file naming convention to fix the wrong prompt model/repo names
- unify naming functions
- add warning msg when the valid input contains _ or accented char
- add/modify test cases for above changes

@agnes512 agnes512 force-pushed the modelNaming branch from a55caf6 to 20d29e6 Jun 8, 2019

@agnes512 agnes512 merged commit 0b2a45b into master Jun 8, 2019

3 of 4 checks passed

coverage/coveralls Coverage decreased (-0.02%) to 91.877%
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

@delete-merged-branch delete-merged-branch bot deleted the modelNaming branch Jun 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.