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

Fixed ignoring npm name and version properties #200

Merged
merged 2 commits into from
Dec 7, 2017
Merged

Fixed ignoring npm name and version properties #200

merged 2 commits into from
Dec 7, 2017

Conversation

szymonbultrowicz
Copy link
Contributor

It looks like current version of GenerateTask.java has a wrong condition when setting npmName and npmVersion properties. I assume it should work like that (if generateNpmPackageJson is set to true):

  1. npmName/npmVersion not set -> use default value
  2. npmName/npmVersion set -> use given value

However, currently it works like that:

  1. npmName/npmVersion not set -> error is being thrown with message that those properties are required
  2. npmName/npmVersion set -> uses default values (ignores the given ones)

This PR fixes that issue.

@vojtechhabarta
Copy link
Owner

You are right, it's wrong.

You are also right that it should generate default value when

  1. the value is not filled and
  2. generateNpmPackageJson is true.

The second part is missing in your PR.

I think that what's needed is just to flip null tests. Then if generateNpmPackageJson is false but value is null then it stays null.

See this in Maven plugin here: https://github.com/vojtechhabarta/typescript-generator/blob/master/typescript-generator-maven-plugin/src/main/java/cz/habarta/typescript/generator/maven/GenerateMojo.java#L578-L579

What do you think?

@szymonbultrowicz
Copy link
Contributor Author

Actually, I think there is no need to check if the flag is true. Without it, the condition is simpler and those values are going to be ignored, anyway. Although, if you think it's better to check it, I'll do it, no problem.

@vojtechhabarta
Copy link
Owner

Currently if generateNpmPackageJson is false the values are NOT silently ignored but Settings.validate() complains that they can only be used when generateNpmPackageJson is true. I know it's rather pedantic 😄 but I wanted to give users detailed feedback when they're configuring typescript-genereator.

See https://github.com/vojtechhabarta/typescript-generator/blob/master/typescript-generator-core/src/main/java/cz/habarta/typescript/generator/Settings.java#L270

@szymonbultrowicz
Copy link
Contributor Author

Got it. Fixed.

@vojtechhabarta vojtechhabarta merged commit 1291a9c into vojtechhabarta:master Dec 7, 2017
@vojtechhabarta
Copy link
Owner

Merged. Thank you very much.

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.

2 participants