Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Encodable and Decodable support for choice elements #119
Encodable and Decodable support for choice elements #119
Changes from 25 commits
749e0fc
ffbb395
77f9699
c128f28
312c4f5
91693e9
b5684f3
5fd8d8f
5bda791
045e07c
a7fb985
b1b6c27
048d0c3
5748eee
c6ee065
e6467d5
ce09102
f09c79d
0414fd8
e1f0c45
0b9c5cc
a8125e2
bf52ca8
fd594fd
a930d00
5a7a64a
c000573
d4bd9f4
4a99e95
7920b72
683cb34
7db9627
c213808
32195c5
8149ead
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this string be split into multiple lines with
"""
to bring it closer to 80 characters, or at least 100?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this looks like a copy-the-style-as-it-is job. Do you want me to tidy up the other instances as well or just stick to the new code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please, the overall cleanup is much appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a strict line length for the repo? The
.swiftlint.yml
declares violations at150
and200
. I've been sticking to 100 out of habit, but I can constrain these to something else if it is so desired.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been trying to get some balance between changing the whole repository and maintaining readability. 80 doesn't work well when indentation is 4 spaces instead of 2, but I decided to stick with 4 to preserve the changes history. You can clean up as much as you don't find tedious. I think last time I tried to tighten the limit in
.swiftlint.yml
(even to 120) it required too many code changes and reformatting XML snippets in the unit tests, which I just decided to postpone, but never got back to it. Maybe if there too many changes of this type, it can be made in a separate PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All seems fair to me! Probably worth waiting for the official swift-format to roll out to act on anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I just touched the long
preconditionFailure
messages.Large diffs are not rendered by default.