Skip to content

NancyFX return collections as arrays to response negotiator#4061

Closed
simo9000 wants to merge 0 commit intoswagger-api:2.3.0from
simo9000:master
Closed

NancyFX return collections as arrays to response negotiator#4061
simo9000 wants to merge 0 commit intoswagger-api:2.3.0from
simo9000:master

Conversation

@simo9000
Copy link
Copy Markdown
Contributor

@simo9000 simo9000 commented Oct 24, 2016

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch 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)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Changed nancyfx template to convert collections to arrays prior to returning from the path delegates. This makes no difference when the request is for json or xml, however if the request is for html, the Nancy response negotiator can't look up view names based with generic collections. Converting the collection to an array prior to returning control to nancy work will allow the view to be discovered by "[]".

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

@simo9000
Copy link
Copy Markdown
Contributor Author

The Travis CI log shows a bunch of failed dependency downloads as the reason this build failed. Looks like some networking problems on their end.

@fehguy
Copy link
Copy Markdown
Contributor

fehguy commented Oct 27, 2016

Yes Travis was having issues earlier. I'll retrigger

@simo9000
Copy link
Copy Markdown
Contributor Author

I just added a considerably bigger change to parameters.mustache. The AbstractCsharpCodegen sets all collections to consist of nullable types yet the parsers collection had entries that returned these types of collections. I reworked the helper methods to be able to parse nullable and nonnullable collections. I also changed the parser loading to return nullable collection types for the types listed in the swagger spec and defined in AbstractCsharpCodegen.

I did this in a way that minimized the LOC increase and didn't make it harder to read. I'm open to refactoring it if need be.

As far as I can tell the some of the other parsers are not reachable without modifying generated code (i.e. TimeSpan, ulong, uint...). I left them along for now.

@wing328 wing328 added this to the v2.2.2 milestone Nov 1, 2016
@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Nov 1, 2016

@simo9000 thanks for the PR. Did you try to rebase on the latest master earlier? I found a lot of commits not belonging to you in this PR and you may want to submit a new PR instead.

Also I would suggest creating a new branch for the change as per git best practices.

cc @mstefaniuk @jimschubert to see if they've any feedback on this PR.

@simo9000
Copy link
Copy Markdown
Contributor Author

simo9000 commented Nov 1, 2016

@wing328 I did not rebase. I pulled from upstream before pushing and creating the PR. I did that to ensure there wouldn't be any conflicts with any commits that were merged while i was working on it.

My apologies for not creating a new branch.

I will create a new branch, cherry-pick my commits from this one, and create a new PR which will be much cleaner looking.

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Nov 1, 2016

@simo9000 no need to apologize and we appreciate your contribution to this project.

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.

3 participants