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

[DART] Add standard generated classes as reserved #8975

Merged

Conversation

sbu-WBT
Copy link
Contributor

@sbu-WBT sbu-WBT commented Mar 15, 2021

A friend create a specification with a model named ApiClient which resulted for me in two classes with the same name which doesn't compile. So I thought it might be handy to mark the default included classes also as reserved.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@swipesight (2018/09) @jaumard (2018/09) @josh-burton (2019/12) @amondnet (2019/12) @sbu-WBT (2020/12) @kuhnroyal (2020/12) @agilob (2020/12)

@@ -164,7 +164,8 @@ public AbstractDartCodegen() {
"File",
"Client",
"Future",
"Response"
"Response",
"StreamedRequest"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also go to the normal dart client as it is part of the http package and not used in the dio generator?

Copy link
Contributor

Choose a reason for hiding this comment

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

This change is in AbstractDartCodegen.java so it affects all dart clients, including dartdio.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea so it should also go to DartClientCodegen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of this, too. But then I saw Response which is also a http class (If I'm not wrong) so I thought I put it there. I have no problem to put it in the other set

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I added Response here because it also exists in Dio (a different class but same name). Same goes for Client.
I think it doesn't hurt here as the http package can basically be considered part of Dart.

@@ -37,6 +38,18 @@
public DartClientCodegen() {
super();

additionalReservedWords.addAll(
Sets.newHashSet(
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about the opposite problem. I don't generate ApiClient class, I would generate only models. This makes ApiClient a non-reserved word, thus produces unexpected results.

The problem exists, but I'm not convinced this is the right solution here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the correct solution, doing this as well in the dio generator. You may have a couple false positives but that is ok I'd say.

Copy link
Contributor Author

@sbu-WBT sbu-WBT Mar 15, 2021

Choose a reason for hiding this comment

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

There is the option to only generate the models? Then just add the classes to the set only if the parameter is not set. Do you mind telling me the option name?

Or if you have another idea just tell us, I'm open to better solutions

Copy link
Contributor

Choose a reason for hiding this comment

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

Or if you have another idea just tell us, I'm open to better solutions

Well, no. It would have to be conditionally added increasing complexity of such solution. I would prefer to see global PR for this problem rather than each generator doing it on their own.

@wing328 is there any DefaultCodeGen solution for this? A set of potentially used keywords like ApiClient to prevent generated files collide with project files. It would have to be managed independently by each code generator and their cmd options.

Looks like big effort to handle some small cases, but correctness of the output should be priority(?).

Copy link
Contributor

Choose a reason for hiding this comment

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

is there any DefaultCodeGen solution for this? A set of potentially used keywords like ApiClient to prevent generated files collide with project files. It would have to be managed independently by each code generator and their cmd options.

The problem is that most other generators/languages have a concept of absolute/fq imports but Dart does not.
So in other generators you can use the import mapping which can be configured as cmd parameters or in the generator class. The dart generators basically miss uses the import mapping, meaning we HAVE TO use it differently due to the absence of the absolute/fq imports concept.

The additionalReservedWords is just something I came up with for the dio generator to solve this in a simple manner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well that is also not fully correct. Dart has named imports import "package:foo:foo.dart" as bar; but I tried this and it fails with a lot of builders/build_runner (json_serializable, built_value).

@agilob
Copy link
Contributor

agilob commented Mar 15, 2021

Im happy to approve it after @kuhnroyal approves it. I think the problem in other comments needs a wider planning for all generators, making it out of scope for your problem @sbu-WBT

@agilob
Copy link
Contributor

agilob commented Mar 15, 2021

@sbu-WBT re-run the tests

@sbu-WBT sbu-WBT force-pushed the dart-default-classes-reserved branch from eacc7f8 to 316bb76 Compare March 15, 2021 22:40
@wing328 wing328 added this to the 5.1.0 milestone Mar 17, 2021
@wing328 wing328 merged commit e9f3b09 into OpenAPITools:master Mar 17, 2021
@sbu-WBT sbu-WBT deleted the dart-default-classes-reserved branch March 18, 2021 21:08
@wing328
Copy link
Member

wing328 commented Mar 22, 2021

@sbu-WBT thanks for the PR, which has been included in the v5.1.0 release: https://twitter.com/oas_generator/status/1373636654024380423

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants