Skip to content

Feature/objc tasks#3391

Merged
wing328 merged 16 commits intoswagger-api:masterfrom
wberger:feature/objc-tasks
Aug 1, 2016
Merged

Feature/objc tasks#3391
wing328 merged 16 commits intoswagger-api:masterfrom
wberger:feature/objc-tasks

Conversation

@wberger
Copy link
Copy Markdown
Contributor

@wberger wberger commented Jul 18, 2016

I propose a change to the objc ApiClient. Instead of internally created and maintained request IDs the API implementation should return NSURLSessionTasks. This change has two advantages:

  • Less internal state
  • These task object provide real cancellation as compared to the request ID based implementation (even if the task is currently running)

Furthermore I changed the configuration object to be protocol based in order to allow for more flexible integration into projects and more complex configuration setups. A default configuration implementation is provided and can be passed in, if no custom implementation is required.

Finally I renamed the ApiClient to ApiSessionManager to go with the AFNetworking wording.

Wolfgang Berger added 16 commits February 12, 2016 16:01
* master: (682 commits)
  fix, tests for swagger-api#2500
  added sample files
  rebuilt sample
  changed to use tag sanitization method
  Update doc to support resteasy
  add petstore yaml
  fix typo, update sinatra to use origianl petstore spec (yaml)
  made timestamp generation enabled by default
  rebuilt with generation timestamp disabled
  rebuilt
  added bootstrap sample
  added script
  made generation timestamp optional
  added bootstrap, renamed sample
  added bootstrap
  updated templates
  add new model in csharp
  minor fix to docstring in csharp
  add new files for perl, php, ruby
  add new files for JS
  ...
…anager-merge

* feature/objc-sessionmanager:
  updates templates with JSONModel workaround
  updates for ISO8601 0.5.1
  updates templates according to https://github.com/NYTimes/objective-c-style-guide
  first version of session manager
…r-merge

* upstream-master: (1081 commits)
  improve ts node enum naming
  update ruby samples
  update ruby sample
  Update README to clean on mvn package
  fix csharp enum naming
  fix double hyphen in c# generator
  Remove more Java String comparison using "=="
  Not compare Java String with "=="
  fix csharp code sample
  feat dart: add pubName to all remaining library parts
  fix[ruby]: Problem with List of Enum
  feat dart: add pubName to library name.
  feat dart: mv basePath to ApiClient
  Add more security samples (swagger-api#3344)
  Leverage Shipable.io to validate mustache templates (swagger-api#3333)
  fix model name "client" issue
  fix https://github.com/swagger-api/swagger-codegen/pull/3313/files#r70178399
  swagger-api#3285 replaced setModelNamePrefix by setModelNameSuffix
  doc: explain how to run tests
  feat: improve / fix deserialization by parsing type String
  ...

Conflicts:
	modules/swagger-codegen/src/main/resources/objc/Object-body.mustache
…ration

* upstream-master:
  add more companies
  added env variable override
  add link to appveyor build
  fix defeault rspec test for ruby, update security petstore
  [Swift] Use ISO-8601 date format
…ration

* upstream-master:
  update JAR URL to 2.2.0
  updated dev versions
  set interface with discriminator as parent
  null check
  updated readme
  updated versions for release
  update java petstore sample
  derive invoker package name from api/modoel package name
…ad of request IDs which greatly reduces state while offering real cancellation, change to a protocol-based configuration with a default implementation, include a workarount for current JSONModel concurrency issues, add missing super call in QueryParamCollection initWithValuesAndFormat
@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jul 19, 2016

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/swagger-api/swagger-codegen/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/swagger-api/swagger-codegen/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jul 19, 2016

cc @mateuszmackowiak

@wberger
Copy link
Copy Markdown
Contributor Author

wberger commented Jul 19, 2016

@wing328 Thank you for pointing this out, I added a secondary e-mail address to my account.

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jul 30, 2016

@wberger I ran the integration test for default and got the following error:

Testing failed:
    Receiver 'SWGConfiguration' for class message is a forward declaration
    No known class method for selector 'sharedConfig'
    Property 'tempFolderPath' cannot be found in forward class object 'SWGConfiguration'
    Receiver type 'SWGConfiguration' for instance message is a forward declaration
    Property 'sslCaCert' cannot be found in forward class object 'SWGConfiguration'
    Property 'sslCaCert' cannot be found in forward class object 'SWGConfiguration'
    Property 'verifySSL' cannot be found in forward class object 'SWGConfiguration'
    Receiver 'SWGConfiguration' for class message is a forward declaration
    No known class method for selector 'sharedConfig'
** TEST FAILED **

For more information about ObjC client's integration test, please refer to https://github.com/swagger-api/swagger-codegen/wiki/FAQ#how-do-i-run-integration-test-with-petstore-objc-api-client

@wing328 wing328 merged commit d4611e4 into swagger-api:master Aug 1, 2016
@wberger
Copy link
Copy Markdown
Contributor Author

wberger commented Aug 2, 2016

@wing328 Thanks for merging! Do you still need input on the failing integration test?

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Aug 2, 2016

@wberger I merged it by mistake. Can you look into the integration test and update the tests accordingly?

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Aug 2, 2016

@wberger It looks like this change is a breaking one (non-backward compatible) so I think what I will do it to rollback this change and target v2.3.0 instead as next release v2.2.1 is a patch release aiming for non-breaking changes only.

@wberger
Copy link
Copy Markdown
Contributor Author

wberger commented Aug 2, 2016

@wing328 I'm currently working on making it (nearly) non-breaking by making the following changes:

  • Rename ApiSessionManager back to ApiClient. This is also a better naming in the Swagger context
  • Create a default initializer for the Api classes

But maybe it is better to put it into 2.3.0 due to the different Default Configuration naming.

@wberger
Copy link
Copy Markdown
Contributor Author

wberger commented Aug 2, 2016

On hindsight, it is a breaking change anyway (due to the api change). So v2.3.0 is the correct target. Shall I create a new PR with my updates, or is it enough to add it to this PR?

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Aug 2, 2016

Reverted via #3501

Please submit another PR for branch 2.3.0 when you've time.

You may also want to review the auto-generated doc to ensure the code samples are correct after your change.

@wberger wberger mentioned this pull request Aug 3, 2016
@wberger
Copy link
Copy Markdown
Contributor Author

wberger commented Aug 3, 2016

@wing328 I have created a new PR #3510 for branch 2.3.0.

@wberger wberger mentioned this pull request Aug 4, 2016
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.

2 participants