Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes HttpResponse html(String) behaviour #412

Merged
merged 3 commits into from
May 28, 2019
Merged

Changes HttpResponse html(String) behaviour #412

merged 3 commits into from
May 28, 2019

Conversation

ChristianSteffens
Copy link
Contributor

This PR makes two changes:

  • Adds CHANGELOG.md and README.md to the Xcode-Project for easy access
  • Changes the behaviour for HttpResponse case html: This case now needs a full html-string (not only the body-part). For compatibility reason we added a htmlBody case as well, that case will only need the body-part then.

We need to provide a full html-string (with a custom head section), unfortunately the existing html case in HttpResonse always provides a fix head-part (that is probably perfectly fine for most cases).

As this PR makes this version incompatible to previous ones in terms of the html response I would certainly understand any vetos. Then again the case is called html and not htmlBody so it is / might be misleading anyhow - so IMHO would recommend changing the behaviour as this PR provides.

@swifter-bot
Copy link

swifter-bot commented May 17, 2019

2 Warnings
⚠️ It seems like you’ve added new tests to the library. If that’s the case, please update the XCTestManifests.swift file running in your terminal the command swift test --generate-linuxmain.
⚠️ If you ran the command swift test --generate-linuxmain in your terminal, please remove the line testCase(IOSafetyTests.__allTests__IOSafetyTests), from public func __allTests() -> [XCTestCaseEntry] in the bottom of the file. For more reference see #366.
1 Message
📖 Hey, @ChristianSteffens 👋.

Generated by 🚫 Danger

CHANGELOG.md Show resolved Hide resolved
@Vkt0r
Copy link
Member

Vkt0r commented May 21, 2019

@ChristianSteffens Thanks for taking the time to create this PR! I think it makes sense to have the ability to provide a full HTML string and not the only the body as you mentioned. This change would break compatibility but I think it's really low compared with the advantage of having both options (body and full HTML).

Can you please update the DemoServer.swift to use the htmlBody instead of the html for the cases in which was used ?

@ChristianSteffens
Copy link
Contributor Author

I updated the tests.

@Vkt0r
Copy link
Member

Vkt0r commented May 24, 2019

@ChristianSteffens Thanks can you please rebase it and get rid of the conflict? I would happily merge it after that.

@ChristianSteffens
Copy link
Contributor Author

Done.

Copy link
Member

@Vkt0r Vkt0r left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks.

@Vkt0r Vkt0r merged commit e531e9d into httpswift:stable May 28, 2019
tomieq pushed a commit to tomieq/swifterfork that referenced this pull request Apr 1, 2021
Changes HttpResponse html(String) behaviour
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants