-
Notifications
You must be signed in to change notification settings - Fork 58
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
Pretty swiftidentifier #61
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
7bc9e74
modify the swiftidentifier filter to accept an enum
djbe 2ccd127
add tests and docs
djbe 5dfe056
changelog entry
djbe f803819
rework pretty filter a bit as discussed
djbe f5d481f
move swiftidentifier code into enum namespace
djbe 6fb9e7a
remove unnecessary check
djbe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Oh, really, does that filter actually uppercase the first character all the time in our code (even since Swift 3)? Shouldn't by anymore I think… (if it's still the case, might be worth using that PR to make the
normal
mode correct again and stop uppercasing while there's not reason to?)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.
That... would be a breaking change TBH. And for what? Types will still need an uppercase first letter, so we'll still need to apply a filter.
That's what I meant to say in the discussion in #30, add enum cases for swift versions, and maybe even for the type of identifier (variable, case, enum, struct, etc...). Maybe not so extensive (not so many options), but could be possible.
Although upper first letter is a decent default.
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.
Well it's debatable. The aim of
swiftIdentifier
is only to make the string valid Swift. Prettifying the string is nice too, but transforming too much means that if we don't want that transformation we have to apply the reverse of that transform afterwards to cancel it… And that reverse transform may not always exist.Just feels odd to over-transform when half of the cases would need it, sure, but the other half doesn't and would need to revert it, so why force to do it in the first place instead of letting it be opt-in is what I mean. The history of this comes back from Swift 2 of course, but we're passed that now.
But indeed that would be a breaking change so maybe let's keep that for a later 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.
The thing is,
snakeToCamelCase
would uppercase the first letter anyway, no way around that.And I'm not talking about swift 2, but even swift 3/4 types need an uppercase first letter. Only variables and cases don't.
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.
Right, Right