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

[aspnetcore] Fix new keyword collection #385

Merged
merged 3 commits into from
Jun 27, 2018

Conversation

etherealjoy
Copy link
Contributor

@etherealjoy etherealjoy commented Jun 23, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Add isMapModel to CodegenModel to allow distinguishing if a Model is inherited from ICollection.
Small refactor in model.mustache to allow removal of some VS hints.

Solves #275

@mandrean @jimschubert
cc core team for DefaultCodegen and CodegenModel Changes
@wing328 @jimschubert @cbornet @jaz-ah @ackintosh @JFCote @jmini

@@ -58,7 +58,7 @@
public Set<String> allMandatory;

public Set<String> imports = new TreeSet<String>();
public boolean hasVars, emptyVars, hasMoreModels, hasEnums, isEnum, hasRequired, hasOptional, isArrayModel, hasChildren;
public boolean hasVars, emptyVars, hasMoreModels, hasEnums, isEnum, hasRequired, hasOptional, isArrayModel, hasChildren, isCollectionModel;
Copy link
Contributor

Choose a reason for hiding this comment

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

(I may be wrong as I'm new to aspnetcore)

How about add isMapModel instead of isCollectionModel?

I think isCollectionModel is a bit complicated: It is ArraySchema and also MapSchema.

To keep our project as simple as possible, I think isMapModel may be better.

For template creator:

  • in case of treating ArraySchema => {{isArrayModel}}
  • in case of treating MapSchema => {{isMapModel}}
  • in case of treating something collection => {{isArrayModel}} & {{isMapModel}}

Assuming that isMapModel exists, it is a good idea to add isCollectionModel as it can be possible handling some collection models transparently. But if we can satify our requirements without it for now, we need to postpone.

Copy link
Contributor

@ackintosh ackintosh left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Looks good to me.

Copy link
Member

@JFCote JFCote left a comment

Choose a reason for hiding this comment

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

LGTM

@JFCote
Copy link
Member

JFCote commented Jun 26, 2018

Will merge this at the end of the day if there is no more comment :)

@JFCote JFCote merged commit 9990ddb into OpenAPITools:master Jun 27, 2018
@etherealjoy etherealjoy deleted the fix_new_keyword_collection branch June 27, 2018 15:46
@jmini jmini added this to the 3.0.3 milestone Jun 27, 2018
@jmini
Copy link
Member

jmini commented Jun 27, 2018

@JFCote: I have added milestone 3.0.3 (we usually do it when we merge a PR)

@JFCote
Copy link
Member

JFCote commented Jun 27, 2018

@jmini Oups sorry, that was my first merge. I've taken note of this and will take care of that next time! Thanks!

@etherealjoy
Copy link
Contributor Author

;)

@jmini
Copy link
Member

jmini commented Jun 27, 2018

@JFCote: No worry and thank you a lot for merging PRs. Please continue to merge stuff you are comfortable with.


An other feedback, in this project we tend to prefer the "Squash and merge" option on GitHub:

GitHub usage

The debate about which option to select is infinite. There are pro and contra for each way.
The main argument I see is that the history is more clean with this option, but I also participate in project using other PR-merge-strategies.

What matters the most to me: we try to all work the same way. This produce a certain homogeneity in the project.

@ackintosh
Copy link
Contributor

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.

None yet

5 participants