Handle value set with setters the same way than with the additionalProperties map

[jmini]

This feature was implemented with #43 and removed with #54.

---

The framework is providing two ways to set values:

• with the additionalProperties map.
• with the setter.

From a usage point of view, setting a value in one way or an other one should create no difference:

Setter:

JavaClientCodegen config = new io.swagger.codegen.languages.java.JavaClientCodegen();
config.setArtifactId("my-artifact-id");
config.setModelPackage("xyz.company.example.model");

Map:

JavaClientCodegen config = new io.swagger.codegen.languages.java.JavaClientCodegen();
config.additionalProperties().put(CodegenConstants.ARTIFACT_ID, "my-artifact-id")
config.additionalProperties().put(CodegenConstants.MODEL_PACKAGE, "xyz.company.example.model");

In my opinion, using setters is more natural, but both way are offered and needs to be supported.

---

Because of this dual way to set value, the Codegen classes need to conciliate both way and ensure that everything is consistent. This is done in processOpts(). The values need to be available in the templates (using {{artifactId}} or {{modelPacakge}} from the previous example), no matter if they were set the map or the setter way.

---

To solve this issue, the commit aa9e90b pushed with PR #54 needs to be reverted.

[jmini]

I have opened a pull request to solve this: #56

[HugoMario]

My concern about the feature in #43, and now in #56, is that we're ignoring how can affect future migrated generators.
I understand that we have a few generators now but the original plan is migrate them all from swagger codegen 3.0.0 branch

Honestly i have not tested how the {{modelPackage}} and {{apiPackage}} values are used on all other generators, but in the case of php client generator. The default value for {{modelPackage}} is \Swagger\Client\Swagger\Client\Model\[ModelName] instead \Swagger\Client\Model\[ModelName].

This happen with just migrate the generator (no extra change related to the values). So i'm just trying to minimize the effort for those generator migration.

I'd like to know wdyt about it, and also would like to know what problem you see to implement #56 in children classes JavaClientCodegen classes instead the base one.

[jmini]

This happen with just migrate the generator (no extra change related to the values). So i'm just trying to minimize the effort for those generator migration.

I am really sad to read this.

There is already a lot of work required to migrate from v2 to v3. Starting with:

• Split between 2 repositories swagger-codegen and swagger-codegen-generators (makes CI complex see CI: integration test problems with latest build #31, and forces to created chained PRs that are not always merged. See [3.0.0] Cleanup work after 'html2' Codegen migration swagger-codegen#7855 which is a follow up of Migrate html2 Codegen #41)
• Template engine change. handlebar instead of mustache
• Changes in CodegenConfig "is*, isNot*, has*, hasNot* properties have been removed from codegen pojo", meaning that some additional changes are requested.
• Incomplete CI for existing migrated generators in the swagger-codegen repo.
• ...

I try to keep this wiki-page up to date: Swagger Codegen migration (swagger codegen generators repository). I am well aware of each of the requested changes.

And the most blocking factor for me: time requested to review and merge PR.

I consider none of the mentioned issue as blockers to move forward and I try to help were I can. See all my contributions of the past weeks.

---

I'd like to know wdyt about it, and also would like to know what problem you see to implement #56 in children classes JavaClientCodegen classes instead the base one.

Of course this can be made as you describe but this is a bad idea in my opinion. modelPackage is defined at DefaultCodegenConfig level. Just like the setModelPackage(String) Method. This class is responsible for doing the check and for setting the appropriate values in the map and in the member.

With your solution: each subclass that rely on this value should implement the check logic (which will be the case in most of the use-cases). This is a wrong separation of concern in Object-Oriented programming.

Now for one specific generator (php-client in your case), the common rule "additional properties map and member values are the same" is not true. The invokerPackage is used as additional prefix (If I understood the code correcly).

For me your solution looks like this:

• Lets keep the generic case wrong, and delegate it to each of the child implementation.
• Facilitate the v2-v3 migration by not changing the "package definition" handling in the php codegen (migration is complicated, see my first part).

In my opinion, moving the "package definition" handling for the super.processOpts(); call is not a big deal at all.
If you merge #56 I can propose a PR for that. And I will add a unit test to demonstrate that nothing was broken.

---

[HugoMario]

come on, this is not to make you feel sad :)

I see your point about OO and you're right about it, so let's keep it that way. I'll try to help you with PR review, sometime is difficult to me because other tasks.

[HugoMario]

btw, thanks again for your help, and sorry for this mess i made o.O

[jmini]

Fixes for the php client generator: #57, the key point is commit 3e96e04.

As you can see: Tests introduced in PhpClientCodegenTest bevore the merge of commit b6724d3 (containing the changes to solve this issue) are still OK. This ensures no regression in package management for the php-client.

[jmini]

Wiki updated with a new section: Package Name handling.