[Swift4] Expanding CodableHelper with added date and JSON customisations - #3365

Merged
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom
pcoltau:feature/swift4_added_codable_customisations
Dec 16, 2019
Merged

[Swift4] Expanding CodableHelper with added date and JSON customisations#3365
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom
pcoltau:feature/swift4_added_codable_customisations

Conversation

@pcoltau

@pcoltaupcoltau commented Jul 15, 2019

Copy link
Copy Markdown
Contributor

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.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

  • Improving performance of CodableHelper by moving the default DateFormatter to a field variable.
  • Added jsonDecoder and jsonEncoder field variables, making the JSON transformations customisable.
  • Removed prettyPrint as parameter from encode function and moved it to the default JSONEncoder instance, in order to allow customisation of the prettyPrint feature.

@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @d-date (2018/03)

@wing328

wing328 commented Jul 29, 2019

Copy link
Copy Markdown
Member

@pcoltau thanks for the PR. Are you free for a quick chat via https://gitter.im (ID: wing328) to discuss this PR?

@wing328wing328 added this to the 4.1.0 milestone Jul 29, 2019
@wing328wing328 modified the milestones: 4.1.0, 4.1.1Aug 9, 2019
@pcoltau

pcoltau commented Aug 22, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@wing328 Sorry for not responding earlier, I've been on vacation :-)

I've updated the PR with the latest from the upstream master and resolved the conflicts.

I'm on gitter (through my github account), in case you would like to discuss the PR.

@wing328wing328 modified the milestones: 4.1.1, 4.1.2Aug 26, 2019
@wing328wing328 modified the milestones: 4.1.2, 4.1.3Sep 11, 2019
@wing328wing328 modified the milestones: 4.1.3, 4.2.0Oct 4, 2019
@pcoltau
pcoltauforce-pushed the feature/swift4_added_codable_customisations branch from 85d4f48 to 5d85ed1CompareOctober 14, 2019 12:08
@pcoltau

pcoltau commented Oct 14, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I've rebased the latest from master onto this branch.

I can see that most of the changes are in the petstore samples, which is a bit surprising. I was expecting that the changes to the petstore sample would only be related to my changes, but that does not seem to be the case..?

wing328
wing328 reviewed Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache Outdated

@wing328wing328Oct 21, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove these spaces in a blank line

@pcoltaupcoltauDec 10, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed.

wing328
wing328 reviewed Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache Outdated

@wing328wing328Oct 21, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove these spaces in a blank line

@pcoltaupcoltauDec 10, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed.

wing328
wing328 reviewed Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache Outdated

@wing328wing328Oct 21, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove these spaces in a blank line

@pcoltaupcoltauDec 10, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed.

@wing328wing328 removed this from the 4.2.0 milestone Oct 30, 2019
4brunu
4brunu reviewed Dec 6, 2019
View reviewed changes

@4brunu4brunu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for you PR, overall it looks really good :)
Could you please take a look at the suggestions and fix the conflicts?

Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache Outdated
Comment on lines 14 to 35

@4brunu4brunuDec 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think about moving those configurations to a central place where they will be more visible?

openapi-generator/modules/openapi-generator/src/main/resources/swift4/APIs.mustache

Lines 9 to 15 in 775cf65

{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}open{{/nonPublicApi}} class {{projectName}}API {
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var basePath = "{{{basePath}}}"
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var credential: URLCredential?
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var customHeaders: [String:String] = [:]
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var requestBuilderFactory: RequestBuilderFactory = AlamofireRequestBuilderFactory()
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var apiResponseQueue: DispatchQueue = .main
}

This way all the configurations are in the same place.

@4brunu4brunuDec 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On the other hand, all the configurations that are present in APIs.mustache are related to API requests... This is more about parsing, so maybe here may be the right place, but it's hard to discover this configuration.
Whats are your thought on this @pcoltau and @wing328?

Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache Outdated
Comment on lines 14 to 15

@4brunu4brunuDec 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of having customDateFormatter and defaultDateFormatter what do you think about having only a public static var dateFormatter: DateFormatter = {}() and if anyone wants to use a different one, just changes this , since it's already a variable?

@4brunu4brunuDec 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The same applies to customJSONDecoder, defaultJSONDecoder and customJSONEncoder, defaultJSONEncoder.

@pcoltaupcoltauDec 10, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The reason for having both a defaultDateFormatter and customDateFormatter, is if someone wanted to, just briefly, change from the defaultDateFormatter to a custom one, and then back again (because one endpoint has a custom date format), then that would be possible.

@pcoltau
pcoltauforce-pushed the feature/swift4_added_codable_customisations branch from 04e74b5 to 9024766CompareDecember 10, 2019 11:35
@4brunu4brunu mentioned this pull request Dec 10, 2019
Closed
5 tasks
@4brunu

4brunu commented Dec 10, 2019

Copy link
Copy Markdown
Contributor

@pcoltau There are some tests that are failing. Can you please check them?

@4brunu4brunu mentioned this pull request Dec 11, 2019
Merged
@pcoltau

pcoltau commented Dec 11, 2019

Copy link
Copy Markdown
ContributorAuthor

I've fixed one test. I'm not sure the rest are related to my changes, but if so, then please let me know what I need to change.

@4brunu

4brunu commented Dec 11, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1

@pcoltau

pcoltau commented Dec 11, 2019

Copy link
Copy Markdown
ContributorAuthor

@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1

Merged! 👍

@4brunu

4brunu commented Dec 11, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

The CI is failing, but now I think it's not related to this PR.
Yesterday I opened one PR #4754 and it was passing on CI.
Today I restarted the CI to see if would fail like this PR, and it is.

@4brunu

4brunu commented Dec 12, 2019

Copy link
Copy Markdown
Contributor

@pcoltau Looks like CI is fixed in master. Could you please merge master into this PR please? Thanks 👍

@wing328
wing328 changed the base branch from master to 4.3.xDecember 13, 2019 10:06
@wing328

wing328 commented Dec 13, 2019

Copy link
Copy Markdown
Member

I believe this is a breaking change so I've updated this PR to target 4.3.x branch instead.

@wing328

wing328 commented Dec 16, 2019

Copy link
Copy Markdown
Member

Shippable failure already fixed in the master.

@wing328
wing328 merged commit f6dbd48 into OpenAPITools:4.3.xDec 16, 2019
@wing328wing328 modified the milestones: 4.2.3, 4.3.0Dec 16, 2019
@pcoltau
pcoltau deleted the feature/swift4_added_codable_customisations branch December 18, 2019 13:12
@wing328

wing328 commented Mar 27, 2020

Copy link
Copy Markdown
Member

@pcoltau thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@4brunu4brunu4brunu left review comments
@wing328wing328wing328 left review comments

Assignees

No one assigned

Labels

Breaking change (with fallback)Client: SwiftEnhancement: General

Projects

None yet

Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pcoltau@wing328@4brunu
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
[Swift4] Expanding CodableHelper with added date and JSON customisations - #3365
Merged
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Dec 16, 2019
Merged
[Swift4] Expanding CodableHelper with added date and JSON customisations#3365
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Conversation
@pcoltau
@pcoltaupcoltau
commented
Jul 15, 2019

Copy link


Copy Markdown

Contributor
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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  •  Filed the PR against the correct branch: master, 4.1.x, 5.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
  • Improving performance of CodableHelper by moving the default DateFormatter to a field variable.
  • Added jsonDecoder and jsonEncoder field variables, making the JSON transformations customisable.
  • Removed prettyPrint as parameter from encode function and moved it to the default JSONEncoder instance, in order to allow customisation of the prettyPrint feature.
@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @d-date (2018/03)
@wing328
wing328
commented
Jul 29, 2019

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR. Are you free for a quick chat via https://gitter.im (ID: wing328) to discuss this PR?
@wing328wing328
added this to the 4.1.0 milestone
Jul 29, 2019
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@pcoltau
pcoltau
commented
Aug 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@wing328 Sorry for not responding earlier, I've been on vacation :-)
I've updated the PR with the latest from the upstream master and resolved the conflicts.
I'm on gitter (through my github account), in case you would like to discuss the PR.
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
85d4f48 to
5d85ed1CompareOctober 14, 2019 12:08
@pcoltau
pcoltau
commented
Oct 14, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've rebased the latest from master onto this branch.
I can see that most of the changes are in the petstore samples, which is a bit surprising. I was expecting that the changes to the petstore sample would only be related to my changes, but that does not seem to be the case..?
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
4brunu
4brunu
reviewed
Dec 6, 2019
View reviewed changes
@4brunu4brunu
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for you PR, overall it looks really good :)

Could you please take a look at the suggestions and fix the conflicts?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
35

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What do you think about moving those configurations to a central place where they will be more visible?
openapi-generator/modules/openapi-generator/src/main/resources/swift4/APIs.mustache

Lines 9 to 15
in
775cf65
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}open{{/nonPublicApi}} class {{projectName}}API {
 {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var basePath = "{{{basePath}}}" 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var credential: URLCredential? 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var customHeaders: [String:String] = [:] 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var requestBuilderFactory: RequestBuilderFactory = AlamofireRequestBuilderFactory() 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var apiResponseQueue: DispatchQueue = .main 
 } 


This way all the configurations are in the same place.
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

On the other hand, all the configurations that are present in APIs.mustache are related to API requests... This is more about parsing, so maybe here may be the right place, but it's hard to discover this configuration.

Whats are your thought on this @pcoltau and @wing328?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
15

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Instead of having customDateFormatter and defaultDateFormatter what do you think about having only a public static var dateFormatter: DateFormatter = {}() and if anyone wants to use a different one, just changes this , since it's already a variable?
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The same applies to customJSONDecoder, defaultJSONDecoder and customJSONEncoder, defaultJSONEncoder.
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The reason for having both a defaultDateFormatter and customDateFormatter, is if someone wanted to, just briefly, change from the defaultDateFormatter to a custom one, and then back again (because one endpoint has a custom date format), then that would be possible.
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
04e74b5 to
9024766CompareDecember 10, 2019 11:35
@4brunu4brunu
mentioned this pull request
Dec 10, 2019
 Closed

5 tasks
@4brunu
4brunu
commented
Dec 10, 2019

Copy link


Copy Markdown

Contributor
@pcoltau There are some tests that are failing. Can you please check them?
@4brunu4brunu
mentioned this pull request
Dec 11, 2019
 Merged

@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
I've fixed one test. I'm not sure the rest are related to my changes, but if so, then please let me know what I need to change.
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
Merged! 👍
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI is failing, but now I think it's not related to this PR.

Yesterday I opened one PR #4754 and it was passing on CI.

Today I restarted the CI to see if would fail like this PR, and it is.
@4brunu
4brunu
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
@pcoltau Looks like CI is fixed in master. Could you please merge master into this PR please? Thanks 👍
@wing328
wing328
changed the base branch from
master
to
4.3.xDecember 13, 2019 10:06
@wing328
wing328
commented
Dec 13, 2019

Copy link


Copy Markdown

Member
I believe this is a breaking change so I've updated this PR to target 4.3.x branch instead.
@wing328
wing328
commented
Dec 16, 2019

Copy link


Copy Markdown

Member
Shippable failure already fixed in the master.
@wing328
wing328
merged commit f6dbd48
into
OpenAPITools:4.3.xDec 16, 2019
@wing328wing328
modified the milestones:
4.2.3, 4.3.0Dec 16, 2019
@pcoltau
pcoltau
deleted the

feature/swift4_added_codable_customisations

branch
December 18, 2019 13:12
@wing328
wing328
commented
Mar 27, 2020

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@4brunu4brunu4brunu left review comments
@wing328wing328wing328 left review comments

Assignees


No one assigned


Labels

Breaking change (with fallback)Client: SwiftEnhancement: General

Projects


None yet


Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pcoltau@wing328@4brunu

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Swift4] Expanding CodableHelper with added date and JSON customisations - #3365
Merged
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Dec 16, 2019
Merged
[Swift4] Expanding CodableHelper with added date and JSON customisations#3365
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Conversation
@pcoltau
@pcoltaupcoltau
commented
Jul 15, 2019

Copy link


Copy Markdown

Contributor
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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  •  Filed the PR against the correct branch: master, 4.1.x, 5.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
  • Improving performance of CodableHelper by moving the default DateFormatter to a field variable.
  • Added jsonDecoder and jsonEncoder field variables, making the JSON transformations customisable.
  • Removed prettyPrint as parameter from encode function and moved it to the default JSONEncoder instance, in order to allow customisation of the prettyPrint feature.
@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @d-date (2018/03)
@wing328
wing328
commented
Jul 29, 2019

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR. Are you free for a quick chat via https://gitter.im (ID: wing328) to discuss this PR?
@wing328wing328
added this to the 4.1.0 milestone
Jul 29, 2019
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@pcoltau
pcoltau
commented
Aug 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@wing328 Sorry for not responding earlier, I've been on vacation :-)
I've updated the PR with the latest from the upstream master and resolved the conflicts.
I'm on gitter (through my github account), in case you would like to discuss the PR.
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
85d4f48 to
5d85ed1CompareOctober 14, 2019 12:08
@pcoltau
pcoltau
commented
Oct 14, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've rebased the latest from master onto this branch.
I can see that most of the changes are in the petstore samples, which is a bit surprising. I was expecting that the changes to the petstore sample would only be related to my changes, but that does not seem to be the case..?
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
4brunu
4brunu
reviewed
Dec 6, 2019
View reviewed changes
@4brunu4brunu
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for you PR, overall it looks really good :)

Could you please take a look at the suggestions and fix the conflicts?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
35

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What do you think about moving those configurations to a central place where they will be more visible?
openapi-generator/modules/openapi-generator/src/main/resources/swift4/APIs.mustache

Lines 9 to 15
in
775cf65
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}open{{/nonPublicApi}} class {{projectName}}API {
 {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var basePath = "{{{basePath}}}" 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var credential: URLCredential? 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var customHeaders: [String:String] = [:] 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var requestBuilderFactory: RequestBuilderFactory = AlamofireRequestBuilderFactory() 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var apiResponseQueue: DispatchQueue = .main 
 } 


This way all the configurations are in the same place.
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

On the other hand, all the configurations that are present in APIs.mustache are related to API requests... This is more about parsing, so maybe here may be the right place, but it's hard to discover this configuration.

Whats are your thought on this @pcoltau and @wing328?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
15

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Instead of having customDateFormatter and defaultDateFormatter what do you think about having only a public static var dateFormatter: DateFormatter = {}() and if anyone wants to use a different one, just changes this , since it's already a variable?
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The same applies to customJSONDecoder, defaultJSONDecoder and customJSONEncoder, defaultJSONEncoder.
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The reason for having both a defaultDateFormatter and customDateFormatter, is if someone wanted to, just briefly, change from the defaultDateFormatter to a custom one, and then back again (because one endpoint has a custom date format), then that would be possible.
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
04e74b5 to
9024766CompareDecember 10, 2019 11:35
@4brunu4brunu
mentioned this pull request
Dec 10, 2019
 Closed

5 tasks
@4brunu
4brunu
commented
Dec 10, 2019

Copy link


Copy Markdown

Contributor
@pcoltau There are some tests that are failing. Can you please check them?
@4brunu4brunu
mentioned this pull request
Dec 11, 2019
 Merged

@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
I've fixed one test. I'm not sure the rest are related to my changes, but if so, then please let me know what I need to change.
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
Merged! 👍
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI is failing, but now I think it's not related to this PR.

Yesterday I opened one PR #4754 and it was passing on CI.

Today I restarted the CI to see if would fail like this PR, and it is.
@4brunu
4brunu
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
@pcoltau Looks like CI is fixed in master. Could you please merge master into this PR please? Thanks 👍
@wing328
wing328
changed the base branch from
master
to
4.3.xDecember 13, 2019 10:06
@wing328
wing328
commented
Dec 13, 2019

Copy link


Copy Markdown

Member
I believe this is a breaking change so I've updated this PR to target 4.3.x branch instead.
@wing328
wing328
commented
Dec 16, 2019

Copy link


Copy Markdown

Member
Shippable failure already fixed in the master.
@wing328
wing328
merged commit f6dbd48
into
OpenAPITools:4.3.xDec 16, 2019
@wing328wing328
modified the milestones:
4.2.3, 4.3.0Dec 16, 2019
@pcoltau
pcoltau
deleted the

feature/swift4_added_codable_customisations

branch
December 18, 2019 13:12
@wing328
wing328
commented
Mar 27, 2020

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@4brunu4brunu4brunu left review comments
@wing328wing328wing328 left review comments

Assignees


No one assigned


Labels

Breaking change (with fallback)Client: SwiftEnhancement: General

Projects


None yet


Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pcoltau@wing328@4brunu

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Swift4] Expanding CodableHelper with added date and JSON customisations - #3365
Merged
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Dec 16, 2019
Merged
[Swift4] Expanding CodableHelper with added date and JSON customisations#3365
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Conversation
@pcoltau
@pcoltaupcoltau
commented
Jul 15, 2019

Copy link


Copy Markdown

Contributor
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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  •  Filed the PR against the correct branch: master, 4.1.x, 5.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
  • Improving performance of CodableHelper by moving the default DateFormatter to a field variable.
  • Added jsonDecoder and jsonEncoder field variables, making the JSON transformations customisable.
  • Removed prettyPrint as parameter from encode function and moved it to the default JSONEncoder instance, in order to allow customisation of the prettyPrint feature.
@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @d-date (2018/03)
@wing328
wing328
commented
Jul 29, 2019

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR. Are you free for a quick chat via https://gitter.im (ID: wing328) to discuss this PR?
@wing328wing328
added this to the 4.1.0 milestone
Jul 29, 2019
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@pcoltau
pcoltau
commented
Aug 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@wing328 Sorry for not responding earlier, I've been on vacation :-)
I've updated the PR with the latest from the upstream master and resolved the conflicts.
I'm on gitter (through my github account), in case you would like to discuss the PR.
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
85d4f48 to
5d85ed1CompareOctober 14, 2019 12:08
@pcoltau
pcoltau
commented
Oct 14, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've rebased the latest from master onto this branch.
I can see that most of the changes are in the petstore samples, which is a bit surprising. I was expecting that the changes to the petstore sample would only be related to my changes, but that does not seem to be the case..?
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
4brunu
4brunu
reviewed
Dec 6, 2019
View reviewed changes
@4brunu4brunu
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for you PR, overall it looks really good :)

Could you please take a look at the suggestions and fix the conflicts?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
35

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What do you think about moving those configurations to a central place where they will be more visible?
openapi-generator/modules/openapi-generator/src/main/resources/swift4/APIs.mustache

Lines 9 to 15
in
775cf65
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}open{{/nonPublicApi}} class {{projectName}}API {
 {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var basePath = "{{{basePath}}}" 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var credential: URLCredential? 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var customHeaders: [String:String] = [:] 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var requestBuilderFactory: RequestBuilderFactory = AlamofireRequestBuilderFactory() 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var apiResponseQueue: DispatchQueue = .main 
 } 


This way all the configurations are in the same place.
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

On the other hand, all the configurations that are present in APIs.mustache are related to API requests... This is more about parsing, so maybe here may be the right place, but it's hard to discover this configuration.

Whats are your thought on this @pcoltau and @wing328?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
15

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Instead of having customDateFormatter and defaultDateFormatter what do you think about having only a public static var dateFormatter: DateFormatter = {}() and if anyone wants to use a different one, just changes this , since it's already a variable?
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The same applies to customJSONDecoder, defaultJSONDecoder and customJSONEncoder, defaultJSONEncoder.
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The reason for having both a defaultDateFormatter and customDateFormatter, is if someone wanted to, just briefly, change from the defaultDateFormatter to a custom one, and then back again (because one endpoint has a custom date format), then that would be possible.
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
04e74b5 to
9024766CompareDecember 10, 2019 11:35
@4brunu4brunu
mentioned this pull request
Dec 10, 2019
 Closed

5 tasks
@4brunu
4brunu
commented
Dec 10, 2019

Copy link


Copy Markdown

Contributor
@pcoltau There are some tests that are failing. Can you please check them?
@4brunu4brunu
mentioned this pull request
Dec 11, 2019
 Merged

@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
I've fixed one test. I'm not sure the rest are related to my changes, but if so, then please let me know what I need to change.
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
Merged! 👍
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI is failing, but now I think it's not related to this PR.

Yesterday I opened one PR #4754 and it was passing on CI.

Today I restarted the CI to see if would fail like this PR, and it is.
@4brunu
4brunu
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
@pcoltau Looks like CI is fixed in master. Could you please merge master into this PR please? Thanks 👍
@wing328
wing328
changed the base branch from
master
to
4.3.xDecember 13, 2019 10:06
@wing328
wing328
commented
Dec 13, 2019

Copy link


Copy Markdown

Member
I believe this is a breaking change so I've updated this PR to target 4.3.x branch instead.
@wing328
wing328
commented
Dec 16, 2019

Copy link


Copy Markdown

Member
Shippable failure already fixed in the master.
@wing328
wing328
merged commit f6dbd48
into
OpenAPITools:4.3.xDec 16, 2019
@wing328wing328
modified the milestones:
4.2.3, 4.3.0Dec 16, 2019
@pcoltau
pcoltau
deleted the

feature/swift4_added_codable_customisations

branch
December 18, 2019 13:12
@wing328
wing328
commented
Mar 27, 2020

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@4brunu4brunu4brunu left review comments
@wing328wing328wing328 left review comments

Assignees


No one assigned


Labels

Breaking change (with fallback)Client: SwiftEnhancement: General

Projects


None yet


Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pcoltau@wing328@4brunu

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
[Swift4] Expanding CodableHelper with added date and JSON customisations - #3365
Merged
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Dec 16, 2019
Merged
[Swift4] Expanding CodableHelper with added date and JSON customisations#3365
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Conversation
@pcoltau
@pcoltaupcoltau
commented
Jul 15, 2019

Copy link


Copy Markdown

Contributor
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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  •  Filed the PR against the correct branch: master, 4.1.x, 5.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
  • Improving performance of CodableHelper by moving the default DateFormatter to a field variable.
  • Added jsonDecoder and jsonEncoder field variables, making the JSON transformations customisable.
  • Removed prettyPrint as parameter from encode function and moved it to the default JSONEncoder instance, in order to allow customisation of the prettyPrint feature.
@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @d-date (2018/03)
@wing328
wing328
commented
Jul 29, 2019

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR. Are you free for a quick chat via https://gitter.im (ID: wing328) to discuss this PR?
@wing328wing328
added this to the 4.1.0 milestone
Jul 29, 2019
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@pcoltau
pcoltau
commented
Aug 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@wing328 Sorry for not responding earlier, I've been on vacation :-)
I've updated the PR with the latest from the upstream master and resolved the conflicts.
I'm on gitter (through my github account), in case you would like to discuss the PR.
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
85d4f48 to
5d85ed1CompareOctober 14, 2019 12:08
@pcoltau
pcoltau
commented
Oct 14, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've rebased the latest from master onto this branch.
I can see that most of the changes are in the petstore samples, which is a bit surprising. I was expecting that the changes to the petstore sample would only be related to my changes, but that does not seem to be the case..?
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
4brunu
4brunu
reviewed
Dec 6, 2019
View reviewed changes
@4brunu4brunu
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for you PR, overall it looks really good :)

Could you please take a look at the suggestions and fix the conflicts?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
35

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What do you think about moving those configurations to a central place where they will be more visible?
openapi-generator/modules/openapi-generator/src/main/resources/swift4/APIs.mustache

Lines 9 to 15
in
775cf65
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}open{{/nonPublicApi}} class {{projectName}}API {
 {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var basePath = "{{{basePath}}}" 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var credential: URLCredential? 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var customHeaders: [String:String] = [:] 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var requestBuilderFactory: RequestBuilderFactory = AlamofireRequestBuilderFactory() 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var apiResponseQueue: DispatchQueue = .main 
 } 


This way all the configurations are in the same place.
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

On the other hand, all the configurations that are present in APIs.mustache are related to API requests... This is more about parsing, so maybe here may be the right place, but it's hard to discover this configuration.

Whats are your thought on this @pcoltau and @wing328?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
15

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Instead of having customDateFormatter and defaultDateFormatter what do you think about having only a public static var dateFormatter: DateFormatter = {}() and if anyone wants to use a different one, just changes this , since it's already a variable?
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The same applies to customJSONDecoder, defaultJSONDecoder and customJSONEncoder, defaultJSONEncoder.
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The reason for having both a defaultDateFormatter and customDateFormatter, is if someone wanted to, just briefly, change from the defaultDateFormatter to a custom one, and then back again (because one endpoint has a custom date format), then that would be possible.
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
04e74b5 to
9024766CompareDecember 10, 2019 11:35
@4brunu4brunu
mentioned this pull request
Dec 10, 2019
 Closed

5 tasks
@4brunu
4brunu
commented
Dec 10, 2019

Copy link


Copy Markdown

Contributor
@pcoltau There are some tests that are failing. Can you please check them?
@4brunu4brunu
mentioned this pull request
Dec 11, 2019
 Merged

@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
I've fixed one test. I'm not sure the rest are related to my changes, but if so, then please let me know what I need to change.
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
Merged! 👍
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI is failing, but now I think it's not related to this PR.

Yesterday I opened one PR #4754 and it was passing on CI.

Today I restarted the CI to see if would fail like this PR, and it is.
@4brunu
4brunu
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
@pcoltau Looks like CI is fixed in master. Could you please merge master into this PR please? Thanks 👍
@wing328
wing328
changed the base branch from
master
to
4.3.xDecember 13, 2019 10:06
@wing328
wing328
commented
Dec 13, 2019

Copy link


Copy Markdown

Member
I believe this is a breaking change so I've updated this PR to target 4.3.x branch instead.
@wing328
wing328
commented
Dec 16, 2019

Copy link


Copy Markdown

Member
Shippable failure already fixed in the master.
@wing328
wing328
merged commit f6dbd48
into
OpenAPITools:4.3.xDec 16, 2019
@wing328wing328
modified the milestones:
4.2.3, 4.3.0Dec 16, 2019
@pcoltau
pcoltau
deleted the

feature/swift4_added_codable_customisations

branch
December 18, 2019 13:12
@wing328
wing328
commented
Mar 27, 2020

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@4brunu4brunu4brunu left review comments
@wing328wing328wing328 left review comments

Assignees


No one assigned


Labels

Breaking change (with fallback)Client: SwiftEnhancement: General

Projects


None yet


Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pcoltau@wing328@4brunu

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Swift4] Expanding CodableHelper with added date and JSON customisations - #3365
Merged
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Dec 16, 2019
Merged
[Swift4] Expanding CodableHelper with added date and JSON customisations#3365
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Conversation
@pcoltau
@pcoltaupcoltau
commented
Jul 15, 2019

Copy link


Copy Markdown

Contributor
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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  •  Filed the PR against the correct branch: master, 4.1.x, 5.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
  • Improving performance of CodableHelper by moving the default DateFormatter to a field variable.
  • Added jsonDecoder and jsonEncoder field variables, making the JSON transformations customisable.
  • Removed prettyPrint as parameter from encode function and moved it to the default JSONEncoder instance, in order to allow customisation of the prettyPrint feature.
@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @d-date (2018/03)
@wing328
wing328
commented
Jul 29, 2019

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR. Are you free for a quick chat via https://gitter.im (ID: wing328) to discuss this PR?
@wing328wing328
added this to the 4.1.0 milestone
Jul 29, 2019
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@pcoltau
pcoltau
commented
Aug 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@wing328 Sorry for not responding earlier, I've been on vacation :-)
I've updated the PR with the latest from the upstream master and resolved the conflicts.
I'm on gitter (through my github account), in case you would like to discuss the PR.
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
85d4f48 to
5d85ed1CompareOctober 14, 2019 12:08
@pcoltau
pcoltau
commented
Oct 14, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've rebased the latest from master onto this branch.
I can see that most of the changes are in the petstore samples, which is a bit surprising. I was expecting that the changes to the petstore sample would only be related to my changes, but that does not seem to be the case..?
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
4brunu
4brunu
reviewed
Dec 6, 2019
View reviewed changes
@4brunu4brunu
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for you PR, overall it looks really good :)

Could you please take a look at the suggestions and fix the conflicts?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
35

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What do you think about moving those configurations to a central place where they will be more visible?
openapi-generator/modules/openapi-generator/src/main/resources/swift4/APIs.mustache

Lines 9 to 15
in
775cf65
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}open{{/nonPublicApi}} class {{projectName}}API {
 {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var basePath = "{{{basePath}}}" 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var credential: URLCredential? 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var customHeaders: [String:String] = [:] 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var requestBuilderFactory: RequestBuilderFactory = AlamofireRequestBuilderFactory() 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var apiResponseQueue: DispatchQueue = .main 
 } 


This way all the configurations are in the same place.
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

On the other hand, all the configurations that are present in APIs.mustache are related to API requests... This is more about parsing, so maybe here may be the right place, but it's hard to discover this configuration.

Whats are your thought on this @pcoltau and @wing328?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
15

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Instead of having customDateFormatter and defaultDateFormatter what do you think about having only a public static var dateFormatter: DateFormatter = {}() and if anyone wants to use a different one, just changes this , since it's already a variable?
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The same applies to customJSONDecoder, defaultJSONDecoder and customJSONEncoder, defaultJSONEncoder.
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The reason for having both a defaultDateFormatter and customDateFormatter, is if someone wanted to, just briefly, change from the defaultDateFormatter to a custom one, and then back again (because one endpoint has a custom date format), then that would be possible.
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
04e74b5 to
9024766CompareDecember 10, 2019 11:35
@4brunu4brunu
mentioned this pull request
Dec 10, 2019
 Closed

5 tasks
@4brunu
4brunu
commented
Dec 10, 2019

Copy link


Copy Markdown

Contributor
@pcoltau There are some tests that are failing. Can you please check them?
@4brunu4brunu
mentioned this pull request
Dec 11, 2019
 Merged

@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
I've fixed one test. I'm not sure the rest are related to my changes, but if so, then please let me know what I need to change.
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
Merged! 👍
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI is failing, but now I think it's not related to this PR.

Yesterday I opened one PR #4754 and it was passing on CI.

Today I restarted the CI to see if would fail like this PR, and it is.
@4brunu
4brunu
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
@pcoltau Looks like CI is fixed in master. Could you please merge master into this PR please? Thanks 👍
@wing328
wing328
changed the base branch from
master
to
4.3.xDecember 13, 2019 10:06
@wing328
wing328
commented
Dec 13, 2019

Copy link


Copy Markdown

Member
I believe this is a breaking change so I've updated this PR to target 4.3.x branch instead.
@wing328
wing328
commented
Dec 16, 2019

Copy link


Copy Markdown

Member
Shippable failure already fixed in the master.
@wing328
wing328
merged commit f6dbd48
into
OpenAPITools:4.3.xDec 16, 2019
@wing328wing328
modified the milestones:
4.2.3, 4.3.0Dec 16, 2019
@pcoltau
pcoltau
deleted the

feature/swift4_added_codable_customisations

branch
December 18, 2019 13:12
@wing328
wing328
commented
Mar 27, 2020

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@4brunu4brunu4brunu left review comments
@wing328wing328wing328 left review comments

Assignees


No one assigned


Labels

Breaking change (with fallback)Client: SwiftEnhancement: General

Projects


None yet


Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pcoltau@wing328@4brunu

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Swift4] Expanding CodableHelper with added date and JSON customisations - #3365
Merged
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Dec 16, 2019
Merged
[Swift4] Expanding CodableHelper with added date and JSON customisations#3365
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Conversation
@pcoltau
@pcoltaupcoltau
commented
Jul 15, 2019

Copy link


Copy Markdown

Contributor
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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  •  Filed the PR against the correct branch: master, 4.1.x, 5.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
  • Improving performance of CodableHelper by moving the default DateFormatter to a field variable.
  • Added jsonDecoder and jsonEncoder field variables, making the JSON transformations customisable.
  • Removed prettyPrint as parameter from encode function and moved it to the default JSONEncoder instance, in order to allow customisation of the prettyPrint feature.
@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @d-date (2018/03)
@wing328
wing328
commented
Jul 29, 2019

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR. Are you free for a quick chat via https://gitter.im (ID: wing328) to discuss this PR?
@wing328wing328
added this to the 4.1.0 milestone
Jul 29, 2019
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@pcoltau
pcoltau
commented
Aug 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@wing328 Sorry for not responding earlier, I've been on vacation :-)
I've updated the PR with the latest from the upstream master and resolved the conflicts.
I'm on gitter (through my github account), in case you would like to discuss the PR.
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
85d4f48 to
5d85ed1CompareOctober 14, 2019 12:08
@pcoltau
pcoltau
commented
Oct 14, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've rebased the latest from master onto this branch.
I can see that most of the changes are in the petstore samples, which is a bit surprising. I was expecting that the changes to the petstore sample would only be related to my changes, but that does not seem to be the case..?
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
4brunu
4brunu
reviewed
Dec 6, 2019
View reviewed changes
@4brunu4brunu
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for you PR, overall it looks really good :)

Could you please take a look at the suggestions and fix the conflicts?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
35

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What do you think about moving those configurations to a central place where they will be more visible?
openapi-generator/modules/openapi-generator/src/main/resources/swift4/APIs.mustache

Lines 9 to 15
in
775cf65
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}open{{/nonPublicApi}} class {{projectName}}API {
 {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var basePath = "{{{basePath}}}" 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var credential: URLCredential? 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var customHeaders: [String:String] = [:] 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var requestBuilderFactory: RequestBuilderFactory = AlamofireRequestBuilderFactory() 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var apiResponseQueue: DispatchQueue = .main 
 } 


This way all the configurations are in the same place.
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

On the other hand, all the configurations that are present in APIs.mustache are related to API requests... This is more about parsing, so maybe here may be the right place, but it's hard to discover this configuration.

Whats are your thought on this @pcoltau and @wing328?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
15

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Instead of having customDateFormatter and defaultDateFormatter what do you think about having only a public static var dateFormatter: DateFormatter = {}() and if anyone wants to use a different one, just changes this , since it's already a variable?
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The same applies to customJSONDecoder, defaultJSONDecoder and customJSONEncoder, defaultJSONEncoder.
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The reason for having both a defaultDateFormatter and customDateFormatter, is if someone wanted to, just briefly, change from the defaultDateFormatter to a custom one, and then back again (because one endpoint has a custom date format), then that would be possible.
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
04e74b5 to
9024766CompareDecember 10, 2019 11:35
@4brunu4brunu
mentioned this pull request
Dec 10, 2019
 Closed

5 tasks
@4brunu
4brunu
commented
Dec 10, 2019

Copy link


Copy Markdown

Contributor
@pcoltau There are some tests that are failing. Can you please check them?
@4brunu4brunu
mentioned this pull request
Dec 11, 2019
 Merged

@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
I've fixed one test. I'm not sure the rest are related to my changes, but if so, then please let me know what I need to change.
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
Merged! 👍
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI is failing, but now I think it's not related to this PR.

Yesterday I opened one PR #4754 and it was passing on CI.

Today I restarted the CI to see if would fail like this PR, and it is.
@4brunu
4brunu
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
@pcoltau Looks like CI is fixed in master. Could you please merge master into this PR please? Thanks 👍
@wing328
wing328
changed the base branch from
master
to
4.3.xDecember 13, 2019 10:06
@wing328
wing328
commented
Dec 13, 2019

Copy link


Copy Markdown

Member
I believe this is a breaking change so I've updated this PR to target 4.3.x branch instead.
@wing328
wing328
commented
Dec 16, 2019

Copy link


Copy Markdown

Member
Shippable failure already fixed in the master.
@wing328
wing328
merged commit f6dbd48
into
OpenAPITools:4.3.xDec 16, 2019
@wing328wing328
modified the milestones:
4.2.3, 4.3.0Dec 16, 2019
@pcoltau
pcoltau
deleted the

feature/swift4_added_codable_customisations

branch
December 18, 2019 13:12
@wing328
wing328
commented
Mar 27, 2020

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@4brunu4brunu4brunu left review comments
@wing328wing328wing328 left review comments

Assignees


No one assigned


Labels

Breaking change (with fallback)Client: SwiftEnhancement: General

Projects


None yet


Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pcoltau@wing328@4brunu

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
[Swift4] Expanding CodableHelper with added date and JSON customisations - #3365
Merged
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Dec 16, 2019
Merged
[Swift4] Expanding CodableHelper with added date and JSON customisations#3365
wing328 merged 11 commits into
OpenAPITools:4.3.xfrom 
pcoltau:feature/swift4_added_codable_customisations
Conversation
@pcoltau
@pcoltaupcoltau
commented
Jul 15, 2019

Copy link


Copy Markdown

Contributor
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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  •  Filed the PR against the correct branch: master, 4.1.x, 5.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
  • Improving performance of CodableHelper by moving the default DateFormatter to a field variable.
  • Added jsonDecoder and jsonEncoder field variables, making the JSON transformations customisable.
  • Removed prettyPrint as parameter from encode function and moved it to the default JSONEncoder instance, in order to allow customisation of the prettyPrint feature.
@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @d-date (2018/03)
@wing328
wing328
commented
Jul 29, 2019

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR. Are you free for a quick chat via https://gitter.im (ID: wing328) to discuss this PR?
@wing328wing328
added this to the 4.1.0 milestone
Jul 29, 2019
@wing328wing328
modified the milestones:
4.1.0, 4.1.1Aug 9, 2019
@pcoltau
pcoltau
commented
Aug 22, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@wing328 Sorry for not responding earlier, I've been on vacation :-)
I've updated the PR with the latest from the upstream master and resolved the conflicts.
I'm on gitter (through my github account), in case you would like to discuss the PR.
@wing328wing328
modified the milestones:
4.1.1, 4.1.2Aug 26, 2019
@wing328wing328
modified the milestones:
4.1.2, 4.1.3Sep 11, 2019
@wing328wing328
modified the milestones:
4.1.3, 4.2.0Oct 4, 2019
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
85d4f48 to
5d85ed1CompareOctober 14, 2019 12:08
@pcoltau
pcoltau
commented
Oct 14, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've rebased the latest from master onto this branch.
I can see that most of the changes are in the petstore samples, which is a bit surprising. I was expecting that the changes to the petstore sample would only be related to my changes, but that does not seem to be the case..?
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
wing328
wing328
reviewed
Oct 21, 2019
View reviewed changes
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated

@wing328wing328Oct 21, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Please remove these spaces in a blank line
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed.
@wing328wing328
removed this from the 4.2.0 milestone
Oct 30, 2019
4brunu
4brunu
reviewed
Dec 6, 2019
View reviewed changes
@4brunu4brunu
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for you PR, overall it looks really good :)

Could you please take a look at the suggestions and fix the conflicts?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
35

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What do you think about moving those configurations to a central place where they will be more visible?
openapi-generator/modules/openapi-generator/src/main/resources/swift4/APIs.mustache

Lines 9 to 15
in
775cf65
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}open{{/nonPublicApi}} class {{projectName}}API {
 {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var basePath = "{{{basePath}}}" 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var credential: URLCredential? 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var customHeaders: [String:String] = [:] 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var requestBuilderFactory: RequestBuilderFactory = AlamofireRequestBuilderFactory() 
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static var apiResponseQueue: DispatchQueue = .main 
 } 


This way all the configurations are in the same place.
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

On the other hand, all the configurations that are present in APIs.mustache are related to API requests... This is more about parsing, so maybe here may be the right place, but it's hard to discover this configuration.

Whats are your thought on this @pcoltau and @wing328?
Comment threadmodules/openapi-generator/src/main/resources/swift4/CodableHelper.mustache
Outdated


Comment on lines

14
 to 
15

@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Instead of having customDateFormatter and defaultDateFormatter what do you think about having only a public static var dateFormatter: DateFormatter = {}() and if anyone wants to use a different one, just changes this , since it's already a variable?
@4brunu4brunuDec 6, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The same applies to customJSONDecoder, defaultJSONDecoder and customJSONEncoder, defaultJSONEncoder.
@pcoltaupcoltauDec 10, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The reason for having both a defaultDateFormatter and customDateFormatter, is if someone wanted to, just briefly, change from the defaultDateFormatter to a custom one, and then back again (because one endpoint has a custom date format), then that would be possible.
@pcoltau
pcoltauforce-pushed
the
feature/swift4_added_codable_customisations
branch
from
04e74b5 to
9024766CompareDecember 10, 2019 11:35
@4brunu4brunu
mentioned this pull request
Dec 10, 2019
 Closed

5 tasks
@4brunu
4brunu
commented
Dec 10, 2019

Copy link


Copy Markdown

Contributor
@pcoltau There are some tests that are failing. Can you please check them?
@4brunu4brunu
mentioned this pull request
Dec 11, 2019
 Merged

@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
I've fixed one test. I'm not sure the rest are related to my changes, but if so, then please let me know what I need to change.
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
@pcoltau
pcoltau
commented
Dec 11, 2019

Copy link


Copy Markdown

ContributorAuthor
@pcoltau I have created a PR on your fork, that fixes the tests. Can you take a look at it please?
pcoltau#1
Merged! 👍
@4brunu
4brunu
commented
Dec 11, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The CI is failing, but now I think it's not related to this PR.

Yesterday I opened one PR #4754 and it was passing on CI.

Today I restarted the CI to see if would fail like this PR, and it is.
@4brunu
4brunu
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
@pcoltau Looks like CI is fixed in master. Could you please merge master into this PR please? Thanks 👍
@wing328
wing328
changed the base branch from
master
to
4.3.xDecember 13, 2019 10:06
@wing328
wing328
commented
Dec 13, 2019

Copy link


Copy Markdown

Member
I believe this is a breaking change so I've updated this PR to target 4.3.x branch instead.
@wing328
wing328
commented
Dec 16, 2019

Copy link


Copy Markdown

Member
Shippable failure already fixed in the master.
@wing328
wing328
merged commit f6dbd48
into
OpenAPITools:4.3.xDec 16, 2019
@wing328wing328
modified the milestones:
4.2.3, 4.3.0Dec 16, 2019
@pcoltau
pcoltau
deleted the

feature/swift4_added_codable_customisations

branch
December 18, 2019 13:12
@wing328
wing328
commented
Mar 27, 2020

Copy link


Copy Markdown

Member
@pcoltau thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@4brunu4brunu4brunu left review comments
@wing328wing328wing328 left review comments

Assignees


No one assigned


Labels

Breaking change (with fallback)Client: SwiftEnhancement: General

Projects


None yet


Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pcoltau@wing328@4brunu