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

Add an option for encoding scalars with newlines to Emitter.Options #406

Merged
merged 6 commits into from
Apr 7, 2024

Conversation

tejassharma96
Copy link
Contributor

Solves #405 by adding a new option for encoding scalars which contain newlines

@acecilia
Copy link
Contributor

Thanks! Looking forward to this. @jpsim wdyt? 🙏

@danibachar
Copy link

+1 for this too

Yams.podspec Outdated
@@ -1,6 +1,6 @@
Pod::Spec.new do |s|
s.name = 'Yams'
s.version = '5.0.6'
s.version = '5.1.0'
Copy link
Owner

Choose a reason for hiding this comment

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

Please don’t touch this file. I’ll bump the version when the time comes to cut a release.

Copy link
Owner

@jpsim jpsim 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! Could you please add a change log entry and rebase? Thanks for your contribution.

@tejassharma96
Copy link
Contributor Author

ok rebased and updated the changelog 🙏

@tejassharma96
Copy link
Contributor Author

older swift versions do not understand any Protocol and the tests are failing because of that:

Encoder.swift:229:45: protocol 'StringProtocol' can only be used as a generic constraint because it has Self or associated type requirements

updated to add a #if swift(>=5.6), which I believe is when support for that was added, and for prior versions just try to cast to String instead of any StringProtocol

@jpsim
Copy link
Owner

jpsim commented Apr 2, 2024

@tejassharma96 just making sure you see the CI failures here.

@tejassharma96
Copy link
Contributor Author

yes, sorry - somewhat distracted. I don't have an environment where I can install old versions of xcode since I'm on sonoma :/ how would you suggest I debug?

@tejassharma96
Copy link
Contributor Author

Ah actually - based on this article, the check should be for 5.7 instead of 5.6:

in Swift 5.7 the requirements around protocols with a Self requirement have been relaxed

Updated the check, could you see if CI succeeds now please?

@tejassharma96
Copy link
Contributor Author

hmmm, one failure still but looks unrelated to this change:

Failing tests:
	YamsTests:
		PerformanceTests.testUsingComposeWithUTF8()
		PerformanceTests.testUsingComposeWithUTF8()

will see if I can repro locally

@jpsim
Copy link
Owner

jpsim commented Apr 4, 2024

CI is all ✅

I’ll merge shortly.

@jpsim jpsim merged commit 029b612 into jpsim:main Apr 7, 2024
52 checks passed
@acecilia
Copy link
Contributor

acecilia commented Apr 7, 2024

Thank you both 🙌

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.

4 participants