Added StringFilters.camelToSnakeCase filter #24

ggrell · 2017-02-22T19:56:58Z

Registered for Stencil Swift extensions
Added test coverage
Updated README.md

…wift extensions. Added test coverage Updated README.md

djbe

Except for the general question about snakecase behaviour, this looks fine to me. Don't forget to add a changelog entry and credit yourself.

djbe · 2017-02-22T20:05:59Z

Sources/Filters.swift

@@ -89,6 +89,11 @@ struct StringFilters {
    }
  }

+  static func camelToSnakeCase(_ value: Any?) throws -> Any? {
+    guard let string = value as? String else { throw FilterError.invalidInputType }
+    return try snakecase(string).lowercased()


Is this to avoid URLChooser --> URL_Chooser?

According to https://en.wikipedia.org/wiki/Snake_case, Hello_world is valid output. No idea if that's what we want though.

Hmm, don't think I've ever seen snake case with caps. The scenario I'm trying to get to is auto-generating json_field_names from camelCase, which tend to be all lower case.

Should I break it out into two different ones? camelToSnakeCase and camelToLowerSnakeCase? Or does Stencil already provides a way to combine something like this: {{ "someTextHere"|camelToSnakeCase|lowercase }}?

I haven't seen it either, I was just wondering what the correct / default behaviour should be. Looking at some libraries in other languages, the default seems to be lowercase, so let's do that 👍

You could always add a parameter to the filter to disable the lowercasing (see for example the stencil join filter http://stencil.fuller.li/en/latest/builtins.html#built-in-filters).

Unfortunately the Wikipedia page doesn't give any definitive rules for upper snake case. For instance, as the StringFilter.snakecase() function is currently written, URLChooser becomes URL_Chooser, not Url_Chooser. I couldn't really find anything that said that'd be right or wrong, so I think for now I'll just stick with that implementation. The filter itself will take one optional boolean parameter for lowercasing, defaults to true.

…default true) for lower casing the string. Updated README to describe filter usage Updated CHANGELOG

…ings and numbers for parameters, so rewrote the filter to always lowercase, except when the arg is either "false", "no" or "0"

djbe

Looks good! Just a small refactor for the boolean parsing stuff

djbe · 2017-02-26T14:52:12Z

README.md

@@ -31,7 +31,8 @@ _TODO: [Write more extension Documentation](https://github.com/SwiftGen/StencilS
 * `swiftIdentifier`: Transforms an arbitrary string into a valid Swift identifier (using only valid characters for a Swift identifier as defined in the Swift language reference)
 * `join`: Deprecated. Will be removed now that the same filter exists in Stencil proper.
 * `lowerFirstWord`
-* `snakeToCamelCase` / `snakeToCamelCaseNoPrefix`
+* `snakeToCamelCase` / `snakeToCamelCaseNoPrefix` /


Maybe remove that trailing /

Yup will do

djbe · 2017-02-26T14:53:14Z

StencilSwiftKit.xcodeproj/xcshareddata/xcschemes/Tests.xcscheme

@@ -8,7 +8,7 @@
      <BuildActionEntries>
         <BuildActionEntry
            buildForTesting = "YES"
-            buildForRunning = "NO"
+            buildForRunning = "YES"


Is there a reason this was changed? I don't think it does much.

Ah, probably AppCode made the change since I was running unit tests. Will revert.

Seems to still be there?

Will fix, I must have checked it in inadvertently again

djbe · 2017-02-26T14:54:14Z

Sources/Filters.swift

+  /// - throws: FilterError.invalidInputType if the value parameter isn't a string
+  static func camelToSnakeCase(_ value: Any?, arguments: [Any?]) throws -> Any? {
+    var toLower = true
+    if arguments.count ==  1 {


It might be useful to refactor out this bit (to parse a boolean parameter). That way you can also simplify your tests.

djbe · 2017-02-26T14:55:02Z

Tests/StencilSwiftKitTests/StringFiltersTests.swift

+    ]
+
+    for (input, expected) in expectations {
+      let trueArgResult = try! StringFilters.camelToSnakeCase(input, arguments: ["true"]) as? String


As mentioned above, it might be better to separate the boolean parsing logic into it's own method, that can then be tested separately.

…unction with its own tests

djbe · 2017-02-26T20:16:27Z

Sources/Filters.swift

+  /// - parameter index: the index in the arguments array
+  /// - returns: true or false if a value was parsed, or nil if it wasn't able to
+  static func parseBool(from arguments: [Any?], index: Int) -> Bool? {
+    guard index + 1 <= arguments.count else {


index < arguments.count

djbe · 2017-02-26T20:17:09Z

Sources/Filters.swift

+    case "true", "yes", "1":
+      return true
+    default:
+      return nil


Should the default here be nil or throwing an exception?

Hmm. I'll refactor. If it's an optional argument (which I tested that Stencil allows for it), it should return nil, otherwise it should throw.

Sounds good!

djbe · 2017-02-26T20:17:54Z

Tests/StencilSwiftKitTests/StringFiltersTests.swift

@@ -133,6 +133,122 @@ class StringFiltersTests: XCTestCase {
    }
  }

+  func testParseBool_WithTrueString() throws {


Might be better to move all these parse tests to a separate XCTestCase.

…onal and required values to be in the arguments. If optional and not found, returns nil. If not optional and not found, throws exception. Separated out parseBool tests into separate test class Added XCTAssertThrows for testing

djbe · 2017-02-27T16:38:33Z

Sources/Filters.swift

@@ -123,7 +129,11 @@ struct StringFilters {
    case "true", "yes", "1":
      return true
    default:
-      return nil
+      if required {


What I meant with my throwing behaviour was more along the lines of, if I pass the parameter someInvalidStuff instead of true or false, that should be considered and thus throw.

I'd change the guard to:

guard index < arguments.count, let boolArg = arguments[index] as? String else { ... required check...

In the switch, you then don't check for required, the default case always throws.

Yup, good eye!

djbe · 2017-02-27T16:41:10Z

Tests/StencilSwiftKitTests/StringFiltersTests.swift

@@ -133,6 +133,77 @@ class StringFiltersTests: XCTestCase {
    }
  }

+  func testCamelToSnakeCase_WithNoArgsDefaultsToTrue() {


You can remove this case, it's covered by the parseBool tests and the 2 cases below this one.

This test covers the ?? true flow (and validated that if no argument given, it'll default to lower case) in the line:
let toLower = try parseBool(from: arguments, index: 0, required: false) ?? true because there's no other test for the no arguments case. The other two camelToSnakeCase tests pass arguments.

True, good call.

djbe

Right, so I've enabled CI builds for forks. Please fix the lint issues, you can always test this locally with rake lint:code and rake lint:tests.

…akeCase

AliSoftware · 2017-03-04T20:24:47Z

Sources/Filters.swift

+
+    let snakeCase = try snakecase(string)
+    if toLower {
+      return snakeCase.lowercased()


There's already a lowercase filter provided by Stencil, so you could already do foo|camelToSnakeCase|lowercase instead of foo|camelToSnakeCase(true).

I think it would be better to keep the lowercasing separate, i.e. let the caller filter thru |lowercase in the template, instead of making that logic in code (and with the risk of repeating lowercasing logic in Swift code).

Besides, it would allow you to simplify this whole PR by not needing this parseBool anymore (I'm surprised that Stencil doesn't provide a way to parse a boolean argument the same way for all filters already btw?).

And I think it would be more understandable to be explicit. Sure you'll lose the "automatic lowercase by default" behavior, that would be unfortunate, but the code would be much simpler (and the risk of regression and inconsistency in the bool argument parsing logic lowered too)

We might need the parseBool logic for other filters we want to refactor (snakeToCamelCaseNoPrefix), but it might then be better to leave that for another PR.

Currently foo|camelToSnakeCase|lowerCase and foo|camelToSnakeCase are equivalent, and I expect that 99% of people will want the default lowercase behavior. You would only need to add a parameter if you wanted snake case with first upper characters, which IMHO will probably be pretty rare. That's why I'd rather keep it as is.

Fair enough.

AliSoftware · 2017-03-04T20:26:52Z

Tests/StencilSwiftKitTests/StringFiltersTests.swift

+
+  func testCamelToSnakeCase_WithFalse() throws {
+    let expectations = [
+            "string": "string",


A little nitpicking, but could you ensure the indentation you use is consistent with the rest of the project? Thx
(Here even in your own code you have different indentation levels)

AliSoftware · 2017-03-04T20:30:10Z

Tests/StencilSwiftKitTests/XCTest+Assertions.swift

+
+import XCTest
+
+public func XCTAssertThrows(_ expression: @autoclosure () throws -> Any?, _ message: @autoclosure () -> String = "",


Could you instead move that into the TestsHelper.swift existing file?

(tbh I was sure we already implemented a similar helper somewhere but maybe it was in another PR?!)

I remember discussing this, but for now we use XCTAssertThrowsError when we want / expect the expression to fail with an exception.

Wow I don't even remember why I ended up writing it. I'll remove it and just use XCTAssertThrowsError instead.

Corrected indentation in new tests.

…akeCase

ggrell · 2017-03-12T00:28:04Z

Code is ready to be merged, thanks!

AliSoftware · 2017-03-12T01:02:00Z

StencilSwiftKit.xcodeproj/xcshareddata/xcschemes/Tests.xcscheme

@@ -8,7 +8,7 @@
      <BuildActionEntries>
         <BuildActionEntry
            buildForTesting = "YES"
-            buildForRunning = "NO"
+            buildForRunning = "YES"


Seems to still be there?

AliSoftware · 2017-03-12T01:09:13Z

Sources/Filters.swift

+  /// - parameter required: If true, the argument is required and function throws if missing.
+  ///                       If false, returns nil on missing args.
+  /// - returns: true or false if a value was parsed, or nil if it wasn't able to
+  static func parseBool(from arguments: [Any?], index: Int, required: Bool = true) throws -> Bool? {


As we're gonna keep this parseBool method, there's no reason for it to be only limited to string filters so no reason to scope it in the StringFilters type. May be worth creating a dedicated type for filter helpers to put it in there (or extend one if there's already such concept in Stencil, haven't checked)

@ggrell Any update on these last small items, or would you prefer I take them up?

AliSoftware · 2017-04-30T17:27:48Z

I'm added the the small missing changes in #35, which will superseed this one so I'm closing it 😉

AliSoftware · 2017-04-30T17:34:51Z

This has now been merged by commit b2f4141

AliSoftware · 2017-05-01T00:07:12Z

Hey @ggrell !

Thanks a lot for contributing to SwiftGen & StencilSwiftKit! 👍
To thank you for your work and for taking time to improve the tool, 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 😉

ggrell · 2017-05-01T09:54:02Z

Got overwhelmed with life lately so didn't have a chance to make those final changes; thank you for wrapping it up! I'd be happy to keep contributing though.

Added StringFilters.camelToSnakeCase filter, registered for Stencil S…

74e6c16

…wift extensions. Added test coverage Updated README.md

djbe requested changes Feb 22, 2017

View reviewed changes

Gyuri Grell and others added 3 commits February 25, 2017 11:18

updated camelToSnakeCase to take a single optional boolean argument (…

7b9be34

…default true) for lower casing the string. Updated README to describe filter usage Updated CHANGELOG

Merge branch 'master' into camelToSnakeCase

b2b6d43

Plugged the code into Sourcery and found that Stencil only parses str…

b715499

…ings and numbers for parameters, so rewrote the filter to always lowercase, except when the arg is either "false", "no" or "0"

djbe requested changes Feb 26, 2017

View reviewed changes

Gyuri Grell added 2 commits February 26, 2017 14:23

Based on PR comments, factored out argument parsing into a separate f…

872c030

…unction with its own tests

Forgot to update comments and test names after renaming function

fd87056

djbe reviewed Feb 26, 2017

View reviewed changes

djbe reviewed Feb 27, 2017

View reviewed changes

Good call in PR comments to simplify parseBool logic.

1089dbe

djbe approved these changes Feb 27, 2017

View reviewed changes

Trigger CI build

852a67b

djbe requested changes Feb 27, 2017

View reviewed changes

Gyuri Grell and others added 3 commits March 2, 2017 11:30

Cleaned up lint issues

1148a85

Merge remote-tracking branch 'origin/camelToSnakeCase' into camelToSn…

b0cdda3

…akeCase

Merge branch 'master' into camelToSnakeCase

a6f82af

djbe approved these changes Mar 4, 2017

View reviewed changes

djbe added the type: enhancement label Mar 4, 2017

AliSoftware requested changes Mar 4, 2017

View reviewed changes

Gyuri Grell added 2 commits March 9, 2017 17:20

Removed XCTest+Assertions file and using XCTAssertThrowsError instead.

07394dd

Corrected indentation in new tests.

Merge remote-tracking branch 'origin/camelToSnakeCase' into camelToSn…

cc3ed98

…akeCase

AliSoftware requested changes Mar 12, 2017

View reviewed changes

djbe mentioned this pull request Apr 2, 2017

SwiftGen 4.2.1 SwiftGen/SwiftGen#286

Closed

18 tasks

djbe added the status: WIP label Apr 16, 2017

Merge branch 'master' into camelToSnakeCase

4672b9a

AliSoftware mentioned this pull request Apr 30, 2017

Added StringFilters.camelToSnakeCase filter #35

Merged

AliSoftware closed this Apr 30, 2017


		import XCTest

		public func XCTAssertThrows(_ expression: @autoclosure () throws -> Any?, _ message: @autoclosure () -> String = "",

Added StringFilters.camelToSnakeCase filter #24

Added StringFilters.camelToSnakeCase filter #24

Conversation

ggrell commented Feb 22, 2017

djbe 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

djbe Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbe 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

ggrell Feb 26, 2017 • edited Loading

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

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

ggrell Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbe Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

ggrell Mar 9, 2017 • edited Loading

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

ggrell commented Mar 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware commented Apr 30, 2017

AliSoftware commented Apr 30, 2017

AliSoftware commented May 1, 2017

ggrell commented May 1, 2017

djbe Feb 24, 2017 •

edited

Loading

ggrell Feb 26, 2017 •

edited

Loading

ggrell Feb 27, 2017 •

edited

Loading

djbe Mar 4, 2017 •

edited

Loading

ggrell Mar 9, 2017 •

edited

Loading