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

Use exact project name for code generation #1122

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

jindong-zhannng
Copy link
Contributor

The default case type conversion rule don't always make sense for abbreviated project names. For example:

{
  "projectName": "FormML",
  "languages": [
    ...
  ],
}

Then generated codes will be like FormMlAstType. But the expectation may be FormMLAstType to keep abbreviations in uppercase.

So I add an option exact to indicate that don't do any change on project name in generation.

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

I propose changing this to always use the exact project name, and terminate with an error during initial validation if the project name is not a valid identifier.

Actually we're already making this conversion in the Yeoman generator:

https://github.com/jindong-zhannng/langium/blob/fc1601c1e2321d3814fd5224e25412de4289d172/packages/generator-langium/src/index.ts#L150-L152

@jindong-zhannng
Copy link
Contributor Author

I propose changing this to always use the exact project name, and terminate with an error during initial validation if the project name is not a valid identifier.

👍. You mean an error from yeoman generator or langium cli?

  const languageName = _.upperFirst(
      _.camelCase(this.answers.rawLanguageName)
  );
  const languageId = _.kebabCase(this.answers.rawLanguageName);

I propose to never do case conversions like these, as it makes users lose control of the final result. We can prompt user to request names on different cases (maybe with suggested value) in project generation.

It's obviously a breaking change, maybe can be shipped with v2.0.0?

@msujew
Copy link
Member

msujew commented Jul 19, 2023

@jindong-zhannng What Miro meant is that the CLI should accept every name accept for those that are not valid identifiers in TypeScript. For example, a Projectname 42 would lead to a non-valid TypeScript identifier. Everything else should be accepted in the CLI. You don't have to change anything in the yeoman generator.

@msujew
Copy link
Member

msujew commented Jul 19, 2023

Interesting, that didn't happen to me for a while. Sometimes GitHub has troubles with rebases and simply deletes the branch. Please create a new PR 👍

@jindong-zhannng
Copy link
Contributor Author

@msujew That because I did some change 😂.

Now I remove the case conversion behavior and add a regex to validate project name.

@jindong-zhannng jindong-zhannng changed the title Add an option to use exact project name Use exact project name for code generation Jul 19, 2023
@spoenemann
Copy link
Contributor

@jindong-zhannng that's perfect! But your change revealed that the requirements example language doesn't meet this criterion. Could you fix the config of that language so npm run langium:generate works without errors?

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Thank you!

@spoenemann spoenemann merged commit dd09c7c into eclipse-langium:main Jul 20, 2023
@spoenemann spoenemann modified the milestones: v1.3.0, v2.0.0 Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants