Skip to content

[SR-7076] Make ContiguousArray Codable #20715

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

Merged
merged 2 commits into from
Jan 15, 2019

Conversation

anayini
Copy link
Contributor

@anayini anayini commented Nov 24, 2018

Implements Encodable and Decodable for ContiguousArray.

Resolves SR-7076.

Implements Encodable and Decodable for ContiguousArray.
@anayini
Copy link
Contributor Author

anayini commented Nov 24, 2018

Trying to run tests locally as well, but having some build issues...might be because I'm on 10.14...

Copy link
Collaborator

@xwu xwu left a comment

Choose a reason for hiding this comment

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

A formatting point: please use two spaces for indents in this file (and throughout the stdlib).

@anayini
Copy link
Contributor Author

anayini commented Nov 24, 2018

@xwu - Thank you for the quick feedback! Looks like I applied the fix then rebased onto the upstream where the @inlinable had been removed.

@anayini
Copy link
Contributor Author

anayini commented Nov 28, 2018

@xwu - Are you able to trigger the tests?

@xwu
Copy link
Collaborator

xwu commented Nov 28, 2018

Sure, but this PR needs to have some tests first before that'd be meaningful, no?

@anayini
Copy link
Contributor Author

anayini commented Nov 28, 2018

@xwu - Yeah figured still made sense to run the smoke tests. I looked around a bit and couldn't find the spot where Codable tests were for Array and what not (wasn't sure there was a great example for the above in CodableTests). Definitely happy to add a test to the appropriate spot if I can get some guidance as to where to put those / examples of similar ones!

@anayini
Copy link
Contributor Author

anayini commented Dec 24, 2018

@airspeedswift - Do we still want to move forward with this or shall I close?

@jrose-apple
Copy link
Contributor

Post-break ping for @airspeedswift and maybe @lorentey.

@moiseev
Copy link
Contributor

moiseev commented Jan 14, 2019

@swift-ci Please test

@moiseev
Copy link
Contributor

moiseev commented Jan 15, 2019

@anayini thanks for implementing this. Do you mind adding a simple test as a follow-up PR?

@anayini
Copy link
Contributor Author

anayini commented Jan 15, 2019

@moiseev can do - can you help point me toward a similar Codable test? Was looking for one in the above comments that I could base my test off of.

@benrimmington
Copy link
Contributor

benrimmington commented Jan 15, 2019

@anayini A simple test would be similar to the existing Locale and TimeZone tests.

You could test ContiguousArray<String> literals with different numbers of elements.

The contents of the TestCodable class and tests dictionary are arranged alphabetically.

@dlbuckley
Copy link
Contributor

dlbuckley commented Jan 16, 2019

@anayini you can see the Codable tests that I've added for the Range conformance in
#19532

The instructions to run the tests can be found here.

I was using a command like utils/build-script --release --test or similar to run the tests when I was adding the conformance for the Range types. It takes a while the first time but subsequent runs are much faster.

@anayini
Copy link
Contributor Author

anayini commented Jan 17, 2019

Thanks @dlbuckley @benrimmington! Will try to get to it this weekend!

benrimmington added a commit to benrimmington/swift that referenced this pull request Jan 29, 2019
millenomi pushed a commit to swiftlang/swift-corelibs-foundation that referenced this pull request Nov 11, 2020
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.

6 participants