Additional filters for String; #54

Antondomashnev · 2017-07-16T20:15:21Z

This PR reflects the issue in Sourcery. First of all, all credits goes to the filter's creators: @ilyapuchka, @krzysztofzablocki, @yageek and @sibljon.

I have some concern of better way to implement helper functions to register filters with one and two arguments, at the moment it's just a copy paste from Sourcery repo.

djbe · 2017-07-16T20:50:27Z

First thing that pops in my mind: We already have a first letter uppercase filter, we call it titlecase (link)

I'll take a look at the other filters during the next week.

Edit: I see you've already removed the upperFirstLetter while I was writing this 😄

Antondomashnev · 2017-07-16T20:52:08Z

@djbe that's right. I've noticed it as well 😄
Another concern after small fixes - the Swiflint has warnings because of Strings filters files length and big tuple in Environment.swift. That is the reason of CI fail.

Antondomashnev · 2017-07-25T11:57:27Z

@djbe any chance you can check it soon? 😄

Liquidsoul

There are still some upperFirstLetter references here which makes the build fails 😉
About the swiftlint errors:

file_length: maybe we could silenced it by adding the swiftlint annotation // swiftlint:disable file_length and plan to split the tests into several files in another PR?
large_tuple: if there cannot be another implementation without it, we need to silence the error just for the line

Liquidsoul · 2017-07-28T20:51:21Z

CHANGELOG.md

@@ -36,6 +36,10 @@ Due to the removal of legacy code, there are a few breaking changes in this new

 ### New Features

+* Added the `contains`, `replace`, `hasPrefix`, `hasSuffix`, `upperFirstLetter`, `lowerFirstLetter` filters for strings


You need to remove upperFirstLetter here.

Good catch, thanks. It's removed in cbfbce5

It seems that you missed the CHANGELOG.md change in the commit. It's not in it 😉

Liquidsoul · 2017-07-28T20:54:54Z

Tests/StencilSwiftKitTests/StringFiltersTests.swift

@@ -285,3 +285,215 @@ extension StringFiltersTests {
    }
  }
 }
+
+extension StringFiltersTests {
+  func testUpperFirstLetter() throws {


Because upperFirstLetter was removed you need to remove this.

Thanks, deletion is in cbfbce5

Liquidsoul · 2017-07-28T21:30:16Z

After testing the code locally, it may not be necessary to address the large_tuple issue. The build seems to pass with just the file_length fix.
BTW, I do not think that swiftlint should show a warning here because this is about a closure, not a tuple 🤔

Liquidsoul · 2017-07-29T17:25:16Z

I have submitted a PR to fix the large_tuple issue in Swiftlint realm/SwiftLint#1725
We will see how it goes 😄

Antondomashnev · 2017-07-29T18:49:18Z

In f1af687 the swiftlint rule to check the file length was disabled 😄

Antondomashnev · 2017-07-29T18:49:42Z

I have submitted a PR to fix the large_tuple issue in Swiftlint realm/SwiftLint#1725

Awesome, thanks for taking care of it 👍

Liquidsoul · 2017-07-29T20:18:03Z

Thank you a lot for all this work! 👏

However, I would suggest to split the changes into several PRs, particularly 595e3aa and ed4477e1.
I think that ed4477e1 is kind of related to #56 and 595e3aa involves a lot of changes that are not related to adding the new filters here.

Liquidsoul · 2017-07-29T20:23:16Z

Sorry for my previous comment, I think looking at just the commits individually made me see something that was not here 😅

Antondomashnev · 2017-07-30T13:30:12Z

@djbe I've renamed titlecase into upperFirstLetter in f3d66af.

djbe · 2017-07-30T13:37:53Z

StencilSwiftKit.xcodeproj/project.pbxproj

@@ -257,7 +257,7 @@
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 			shellPath = /bin/sh;
-			shellScript = "if [[ ! $CI ]]; then\n    rake lint:code\n    rake lint:tests\nfi";
+			shellScript = "";


Why was this removed?

By mistake. Rollback in fbd84a0

djbe · 2017-07-30T13:38:52Z

Documentation/filters-strings.md

@@ -114,7 +114,7 @@ https://developer.apple.com/library/content/documentation/Swift/Conceptual/Swift
 | `42hello`  | `_42hello` |
 | `some$URL` | `Some_URL` |

-## Filter: `titlecase`
+## Filter: `upperFirstLetter`


I'd add a note here that titlecase still exists.

Makes sense. Added in 84379ce

djbe · 2017-07-30T13:39:48Z

Sources/Filters+Strings.swift

@@ -60,8 +88,21 @@ extension Filters {
      let transformed = String(scalars[start..<idx]).lowercased() + String(scalars[idx..<scalars.endIndex])
      return transformed
    }
-
+
+    /// Deprecated in favor of `upperFirstLetter`
    static func titlecase(_ value: Any?) throws -> Any? {


Just remove this function, and point the registerFilter call to the function below.

Done in 81aefea

djbe · 2017-07-30T13:40:09Z

Sources/Environment.swift

@@ -20,6 +20,12 @@ public extension Extension {
    registerFilter("snakeToCamelCase", filter: Filters.Strings.snakeToCamelCase)
    registerFilter("swiftIdentifier", filter: Filters.Strings.swiftIdentifier)
    registerFilter("titlecase", filter: Filters.Strings.titlecase)


Point this to the new upperFirstLetter function.

Yep, good point. See 81aefea

djbe · 2017-07-30T13:42:23Z

Nice! We might want to add an entry to the Migration Guide for the titlecase change. The next version should be 2.1.0.

AliSoftware · 2017-08-13T19:46:19Z

I think we can integrate that in the next 2.1.0 👍

AliSoftware · 2017-08-13T20:06:59Z

Btw @Antondomashnev to thank you for your work and for taking time to improve the lib, I've invited you to join SwiftGen's GitHub organization's "CoreContributors" team 😃

No pressure to accept nor any obligation in that! It will just give you the ability to help triage issues or push directly branches to the repo (instead of your fork)… in case you want to contribute again in the future 😉

For more information, read the COMMUNITY.md and CONTRIBUTING.md documents, and if you have any questions don't hesitate to reach out!

Antondomashnev · 2017-08-13T21:44:53Z

@AliSoftware thanks, I'd try to be any help, since we're actively using some SwiftGen projects in Sourcery and I also use some in my OSS 🚀

Antondomashnev · 2017-08-15T18:36:59Z

@AliSoftware @djbe @Liquidsoul may I rebase it into master as it's approved?

AliSoftware · 2017-08-15T18:37:55Z

Would be perfect 👍

Antondomashnev · 2017-08-15T18:40:50Z

@AliSoftware apparently...

AliSoftware · 2017-08-15T18:44:50Z

Sources/Filters+Strings.swift

    static func swiftIdentifier(_ value: Any?) throws -> Any? {
      guard let value = value as? String else { throw Filters.Error.invalidInputType }
      return StencilSwiftKit.swiftIdentifier(from: value, replaceWithUnderscores: true)
    }

+    /* Lowers the first letter of the string
+     * e.g. "People picker" gives "people picker", "Sports Stats" gives "sports Stats"


Why the inconsistency of comments style compared to the rest which properly use ///?

No reason 😄 it's updated in 129ab6d

Liquidsoul · 2017-08-15T18:58:15Z

@Antondomashnev you've been given push access to the repository (you can push your branches to the repo and you can manage the issues) but push to ˋmaster` must be done by the Core team 😉

Liquidsoul · 2017-08-15T18:59:33Z

In other words, you need to do your rebase manually 😅

AliSoftware · 2017-08-15T19:03:44Z

@Antondomashnev I've added you as contributor to the SwiftGenKit repo hopefully it will give you the rights you need

AliSoftware · 2017-08-15T19:11:04Z

Codebeat isn't gonna go green because we increased the size of the filters file too much. I'm gonna merge it anyway but a new PR on top of this, splitting the filters file in multiple Swift files, would be appreciated 😉

Antondomashnev · 2017-08-15T19:28:36Z

@Liquidsoul thanks for the clarification 👍

Antondomashnev added 8 commits July 16, 2017 16:16

Add upperFirstLetter filter;

3b7b658

Add lowerFirstLetter filter;

6504003

Add contains filter;

80aa935

Add hasPrefix filter;

fd18266

Add hasSuffix filter;

4bd710d

Add replace filter;

ae5cc24

Register filters in Stencil;

9e8aecc

Update changelog;

f1d59d6

Antondomashnev changed the title ~~Additional filters for String;~~ Additional filters for String; [WIP] Jul 16, 2017

djbe added the type: enhancement label Jul 16, 2017

djbe added this to the Nice To Have milestone Jul 16, 2017

Antondomashnev changed the title ~~Additional filters for String; [WIP]~~ Additional filters for String; Jul 16, 2017

Liquidsoul requested changes Jul 28, 2017

View reviewed changes

Antondomashnev added 6 commits July 29, 2017 21:02

Fix swiftlint warnings;

595e3aa

Remove upperFirstLetter in favor of existed titlecase;

2857547

Update documentation;

ec6341e

Remove upperFirstLetter;

9efdd5f

Disable file_length rule for strings filters test;

a8023bb

Make SPM happy;

ed477e1

Antondomashnev force-pushed the additional_filters branch from 6035c1e to ed477e1 Compare July 29, 2017 19:02

Remove upperFirstLetter from CHANGELOG.md

11f1f00

djbe reviewed Jul 30, 2017

View reviewed changes

Antondomashnev added 4 commits July 30, 2017 15:42

Rollback swiftlint run phase;

fbd84a0

Note in documentation that titlecase is still available, but deprecated;

84379ce

Remove titlecase function;

81aefea

Fix Swiftlint;

57429a3

AliSoftware approved these changes Aug 13, 2017

View reviewed changes

AliSoftware modified the milestones: 2.1.0, Nice To Have Aug 13, 2017

Merge branch 'master' into additional_filters

5d20867

AliSoftware reviewed Aug 15, 2017

View reviewed changes

Bring consistency to comments in Filters+Strings;

129ab6d

AliSoftware merged commit 00882eb into SwiftGen:master Aug 15, 2017

Antondomashnev deleted the additional_filters branch August 15, 2017 19:28

Antondomashnev mentioned this pull request Aug 15, 2017

Reduce complexity of Strings filters #58

Closed

		@@ -36,6 +36,10 @@ Due to the removal of legacy code, there are a few breaking changes in this new

		### New Features

		* Added the `contains`, `replace`, `hasPrefix`, `hasSuffix`, `upperFirstLetter`, `lowerFirstLetter` filters for strings

Additional filters for String; #54

Additional filters for String; #54

Conversation

Antondomashnev commented Jul 16, 2017 • edited Loading

djbe commented Jul 16, 2017 • edited Loading

Antondomashnev commented Jul 16, 2017

Antondomashnev commented Jul 25, 2017

Liquidsoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Liquidsoul commented Jul 28, 2017

Liquidsoul commented Jul 29, 2017

Antondomashnev commented Jul 29, 2017

Antondomashnev commented Jul 29, 2017

Liquidsoul commented Jul 29, 2017 • edited Loading

Liquidsoul commented Jul 29, 2017

Antondomashnev commented Jul 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbe commented Jul 30, 2017

AliSoftware commented Aug 13, 2017

AliSoftware commented Aug 13, 2017

Antondomashnev commented Aug 13, 2017

Antondomashnev commented Aug 15, 2017

AliSoftware commented Aug 15, 2017

Antondomashnev commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Liquidsoul commented Aug 15, 2017

Liquidsoul commented Aug 15, 2017

AliSoftware commented Aug 15, 2017

AliSoftware commented Aug 15, 2017

Antondomashnev commented Aug 15, 2017

Antondomashnev commented Jul 16, 2017 •

edited

Loading

djbe commented Jul 16, 2017 •

edited

Loading

Liquidsoul commented Jul 29, 2017 •

edited

Loading