Splitting of Filters+Strings #63

Antondomashnev · 2017-08-20T16:40:17Z

This resolves the #58, naming selection was pretty difficult, but so far it looks ok to me. @AliSoftware @djbe if you have better names - I'd be happy to change it 😄

djbe · 2017-08-20T16:50:55Z

Sources/Filters+Strings+Lettercase.swift

+    ///
+    /// - Parameter string: The string to titleCase
+    /// - Returns: The string with its first character uppercased, and the rest of the string unchanged.
+    private static func titlecase(_ string: String) -> String {


Rename this to upperFirstLetter, titlecase is the old name for this filter/function.

I wonder why that titlecase name came back up here, did you copy/paste the code from before the last commits which changed that name when you started working on that split? Or did we miss something when doing our latest merges and let that slip?

@AliSoftware when I cloned the repo today the initial state was 49af697

@AliSoftware I think you messed up something as we're still using titlecase 😄
But we maybe can remove it and use only upperFirstLetter as @djbe commented.

In c4686c3 it's removed, but I had to create a function upperFirstLetter that doesn't throw and takes String parameter to be able to use in:

let comps = string.components(separatedBy: "_") unprefixed = comps.map { upperFirstLetter($0) }.joined(separator: "")

djbe · 2017-08-20T16:52:37Z

Sources/Filters+Strings+Lettercase.swift

+    ///   - string: the value to be processed
+    ///   - stripLeading: if false, will preserve leading underscores
+    /// - Returns: the camel case string
+    static func snakeToCamelCase(_ string: String, stripLeading: Bool) throws -> String {


This should probably be moved to private, except that swiftIdentifier depends on it.

Should we have a clearer separation between the Stencil filter functions (that accept and return Any), and the actual filter methods (that accept and return String + typed arguments)?

Yeah, maybe internal real functions should be separate to functions having a Filter signature and being used in registerFilter… but then where should we put that static func snakeToCamelCase method?

Regarding the:

Should we have a clearer separation between the Stencil filter functions (that accept and return Any), and the actual filter methods (that accept and return String + typed arguments)?

I wanna ask if it makes sense for you to validate input not for string, but for string convertable. That would actually be a benefit for Sourcery and simplify the template writing 😄

@Antondomashnev so this then?

guard let value = value as? CustomStringConvertible else {...} let string = value.description

Seems like a good idea to me, we don't care what the data is as long as we can safely convert it to a string.

@djbe exactly.
Cool, I guess it worth another PR, as to not mess up things 😄

djbe · 2017-08-20T16:55:35Z

While we're at it, maybe check on codebeat if there are other changes we might want to apply:
https://codebeat.co/projects/github-com-swiftgen-stencilswiftkit-master/report_classes/swift-Filters-Strings
Note that some issues are already resolved by this PR (see the codebeat CI step).

AliSoftware · 2017-08-20T18:24:01Z

Pods/Pods.xcodeproj/project.pbxproj

@@ -385,7 +387,9 @@
 				2E92B989035E25169DF9300616F23AAB /* Environment.swift */,
 				0C6D246B203676EC17FA9473D97CB418 /* Filters.swift */,
 				02B0A9A1128124B1A17D5AB1E23A0A45 /* Filters+Numbers.swift */,
-				47DB41142F62E8B5174F3DF281DCD5E3 /* Filters+Strings.swift */,
+				6D1EF02F1F49EC7C00EA91EF /* Filters+Strings+Lettercase.swift */,
+				6D1EF02B1F49E85D00EA91EF /* Filters+Strings+Mutations.swift */,


those short UUIDs intrigue me… is this a new thing in Xcode 9, shorter UUIDs for file references in Xcodeprojs? 🤔

It happens when you add the files manually to the pods project. When you run pod install you get the longer UUIDs.

Thanks @djbe didn't know it. @AliSoftware I used Xcode 8.x but added the, manually.

AliSoftware · 2017-08-20T18:25:58Z

Sources/Filters+Strings+Boolean.swift

+import Stencil
+
+extension Filters {
+  enum Strings {


Mmmh. There's no particular reason why the enum would be declared in +Boolean and extension be used in the other files… But is it worth having a Filters+Strings.swift which would only declare an empty enum Strings{} just so all Filters+Strings+Whatever.swift subsequent file are consistent and all use extension Filters.Strings?

I like the idea, I think it may be clearer for developer, that there is a "base" file.

AliSoftware · 2017-08-20T18:27:48Z

Sources/Filters+Strings+Lettercase.swift

+    ///   - string: the value to be processed
+    ///   - stripLeading: if false, will preserve leading underscores
+    /// - Returns: the camel case string
+    static func snakeToCamelCase(_ string: String, stripLeading: Bool) throws -> String {


Yeah, maybe internal real functions should be separate to functions having a Filter signature and being used in registerFilter… but then where should we put that static func snakeToCamelCase method?

AliSoftware · 2017-08-20T18:28:59Z

Sources/Filters+Strings+Lettercase.swift

+    ///
+    /// - Parameter string: The string to titleCase
+    /// - Returns: The string with its first character uppercased, and the rest of the string unchanged.
+    private static func titlecase(_ string: String) -> String {


I wonder why that titlecase name came back up here, did you copy/paste the code from before the last commits which changed that name when you started working on that split? Or did we miss something when doing our latest merges and let that slip?

AliSoftware · 2017-08-20T18:36:01Z

@Antondomashnev you might want to (1) rebase your branch on top of master now that we recently released SwiftGenKit 2.1.0 (as of minutes ago), and also double and triple-check that what you did cut&paste in separate files didn't get changed by recent commits, making you miss those changes (see the titlecase case) and risking introducing regressions.

Otherwise I'm ok with the names (or rather I can't think of anything better at the moment :-p), thanks for the PR :)

djbe · 2017-08-20T19:08:52Z

I think the code is based on almost the most recent code. titlecase was an old internal method from before the filter got renamed. I think it got missed during the rename and addition of new filters.

Antondomashnev · 2017-08-21T11:59:38Z

@AliSoftware @djbe the build is failing because the file Filters+Strings has not been compiled yet, when the Filters+Strings+... are compiling. I've worked around it with build and test from Xcode, but haven't found solution yet for SPM. If you have an idea that would be helpful.

The simplest way to fix it would be to keep the decalaration of enum Strings as it was in Filters+Strings+Boolean but I do like much more the separate file with declaration.

AliSoftware · 2017-08-21T12:01:39Z

I'm not sure I understand how, in the case where the enum is in Filters+Strings it might not be compiled before, but when it's in Filters+Strings+Boolean it's guaranteed to be compiled before Filters+Strings+Lettercase for example?

djbe · 2017-08-21T12:03:33Z

@Antondomashnev Is it because you added the files to the pods project manually? Order really shouldn't matter. Try running bundle exec pod install.

Antondomashnev · 2017-08-21T12:03:52Z

@AliSoftware I don't understand as well.

But as I wrote in the comment above:

The order of compile sources must be like that, so Filters+Strings.swift is compiled first 😄

Antondomashnev · 2017-08-21T12:04:38Z

@djbe tried it, didn't help. Also tried to remove references for these files, add them again and bundle exec pod install, still no luck.

                31BF3597506A8E41463703B4BA2D71C0 /* Filters+Strings+Boolean.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A3294DE3EE416417CD5A353929BF3A3 /* Filters+Strings+Boolean.swift */; };
		3378EE87DA9FB9454669D5D5191B4E98 /* StencilSwiftKit-umbrella.h in Headers */ = {isa = PBXBuildFile; fileRef = 4DD3B54D1D2C564D5BA9E996D9CD9883 /* StencilSwiftKit-umbrella.h */; settings = {ATTRIBUTES = (Public, ); }; };
		3CFC4A8C135BD9D5BB28AFB16945E761 /* Parameters.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8454F301FD8C9C5EB0330C86D7159347 /* Parameters.swift */; };
		4049984A2C747903582DA0EEDBC1C5DD /* StencilSwiftKit-dummy.m in Sources */ = {isa = PBXBuildFile; fileRef = 81075406C503A904C5D107F68764358A /* StencilSwiftKit-dummy.m */; };
		42E2D5056604852C8C89AE1FCD112B0A /* FilterTag.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3D9976A9FB02F6316003996E10007AEE /* FilterTag.swift */; };
		45C9D21646ED334EA4C9B1DBC549D57E /* Filters+Numbers.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9227E56C42EDA867A814F83D87325E4C /* Filters+Numbers.swift */; };
		486756099E4375C92AEEDD9DBC50C6A6 /* StencilSwiftTemplate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 40BFBD138B4E950E8D24A35620CE4CCC /* StencilSwiftTemplate.swift */; };
		5245ABB6A66FF1DEC56583EF468DD7AA /* PathKit-umbrella.h in Headers */ = {isa = PBXBuildFile; fileRef = C62389B5756FDBA46714335EE5B2079D /* PathKit-umbrella.h */; settings = {ATTRIBUTES = (Public, ); }; };
		539BA0FADD1A22A8319F23AC2757AB0A /* SetNode.swift in Sources */ = {isa = PBXBuildFile; fileRef = ADE4751F2E4F5541BF77BAB13BE069CF /* SetNode.swift */; };
		5D5EC19E156C47504B777C0EA9DD7337 /* NowTag.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0D3CBAF672FD3CA25D7EE1A8E93A6F07 /* NowTag.swift */; };
		5DEF23C8870F6D838AF561730C58977B /* Lexer.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31A007C042C580A003C9B6DB5AD1B534 /* Lexer.swift */; };
		6272EE69D3F5DE1DF030E2B979A19909 /* PathKit.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5B85A6D36EC20031BC5814F8D8D514E8 /* PathKit.swift */; };
		640F760024085F7314950766AD350F39 /* Filters.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2C00655023ABE55F6948E0BFBEB532BC /* Filters.swift */; };
		6B27B2AFB03689DC6884F270293F7C4C /* Filters+Strings.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4ACD468B2E8B13C3250DB2C01EE06C5D /* Filters+Strings.swift */; };

@djbe you can see that Filters+Strings+Boolean.swift is before Filters+Strings.swift, and it's because it seems to be sorted by UUID at least it looks like.

Antondomashnev · 2017-08-21T21:11:56Z

@djbe @AliSoftware order definitely doesn't matter, but SPM still fails 😄

AliSoftware · 2017-08-21T21:13:43Z

Does SPM uses -WMO? Or if not any way to enable it? I wonder if that would make a difference, forcing to compile it as a whole…

Antondomashnev · 2017-08-21T21:16:27Z

@AliSoftware do you mean whole module optimisation?

AliSoftware · 2017-08-21T21:16:54Z

Yup

Antondomashnev · 2017-08-21T21:21:57Z

@AliSoftware seems it does use it in release build and swift build -c release also doesn't work:

Also the Swift Package Manager compiles with whole-module optimizations in release builds.

Antondomashnev · 2017-08-22T14:37:58Z

Yeah, I've made it working. So it seems for the SPM the file names are still matter and I think it's somehow sorted so the Filters+Strings.swift goes after Filter+Strings+....swift.
I've renamed Filters+Strings.swift to Filters+Strings+Base.swift so if we sort it alphabetically which is a guess from my side it becomes the first through all of extensions.

AliSoftware · 2017-08-22T14:39:54Z

Ok, that's cool… but it's still worth a JIRA in bugs.swift.org to me though, as I think that shouldn't be the case 😉

Antondomashnev · 2017-08-22T15:36:12Z

@AliSoftware very valid point, create one. Btw I still can not merge the PRs, maybe master is protected with some rules.

AliSoftware · 2017-08-22T15:37:23Z

Yup, only the CoreTeam have right to merge into master 😉

djbe reviewed Aug 20, 2017

View reviewed changes

AliSoftware reviewed Aug 20, 2017

View reviewed changes

Antondomashnev added 5 commits August 20, 2017 22:15

Split Filters+Strings files;

8fb4c0c

Small cleanup;

a696b55

Update changelog;

2304150

Create a base Filters+Strings;

73ca5da

Remove titlecase function;

c4686c3

Antondomashnev force-pushed the reduce_strings_filters branch from 15ebab8 to c4686c3 Compare August 20, 2017 20:45

Antondomashnev added 4 commits August 20, 2017 22:52

Fix Extension.registerStencilSwiftExtensions codebeat issue;

3f2af0d

Fix Parameters.parse codebeat issue;

58fd00d

Fix public Parameters.parse codebeat issue;

9b979f4

Fix swiftlint warnings;

362f5bc

Workaround the spm build issues;

c4852fd

AliSoftware merged commit b6841ce into master Aug 23, 2017

AliSoftware deleted the reduce_strings_filters branch August 23, 2017 07:41

Antondomashnev mentioned this pull request Aug 29, 2017

Parse string function #65

Merged

AliSoftware mentioned this pull request Mar 17, 2018

Release/2.4.0 #77

Merged

Splitting of Filters+Strings #63

Splitting of Filters+Strings #63

Conversation

Antondomashnev commented Aug 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Antondomashnev Aug 20, 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

djbe Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Antondomashnev Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

djbe commented Aug 20, 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

Antondomashnev Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware commented Aug 20, 2017

djbe commented Aug 20, 2017

Antondomashnev commented Aug 21, 2017

AliSoftware commented Aug 21, 2017 • edited Loading

djbe commented Aug 21, 2017

Antondomashnev commented Aug 21, 2017

Antondomashnev commented Aug 21, 2017 • edited Loading

Antondomashnev commented Aug 21, 2017

AliSoftware commented Aug 21, 2017

Antondomashnev commented Aug 21, 2017 • edited Loading

AliSoftware commented Aug 21, 2017

Antondomashnev commented Aug 21, 2017

Antondomashnev commented Aug 22, 2017

AliSoftware commented Aug 22, 2017

Antondomashnev commented Aug 22, 2017

AliSoftware commented Aug 22, 2017

Antondomashnev Aug 20, 2017 •

edited

Loading

djbe Aug 20, 2017 •

edited

Loading

Antondomashnev Aug 20, 2017 •

edited

Loading

Antondomashnev Aug 20, 2017 •

edited

Loading

AliSoftware commented Aug 21, 2017 •

edited

Loading

Antondomashnev commented Aug 21, 2017 •

edited

Loading

Antondomashnev commented Aug 21, 2017 •

edited

Loading