Skip to content
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

Laziness in action! #71

Closed
wants to merge 11 commits into from
Closed

Laziness in action! #71

wants to merge 11 commits into from

Conversation

ALJCepeda
Copy link

Lazy chained methods:
step() - Consumes current method in chain
stepBackward() - Decrements then consumes method in chain
walk() - Invoke current method then increment chain
walkBackward() - Decrements then invokes current method
resetChain() - Goes to beginning of chain
endChain() - Goes to end of chain
invokeAll() - Evaluates result of entire chain
invokeRest() - Evaluates result of remaining methods on chain relative to current position in chain
hasStep() - Checks if any methods can be consumed from current position in chain
countSteps() - Returns how many methods can be consumed from current position
hasStepBackward() -> Checks if any methods can be consumed by decrementing chain
countBackwardSteps() -> Returns how many methods can be consumed by decrementing chain
hasChain() - Checks if any methods are available to be evaluated.
countChain() - Returns how many methods are available to be evaluated

Lazy iterator:
Type: Dollar.Iterator
next() - Invokes action on next element in collection (current then increment)
previous() - Invoked action on previous element in collection (decrement then current)
cycle() - Invokes action while cycling through elements in collection
cycleBackwards() - Invokes continuously while decrementing through collection
toFirstObject() - Moves position of iterator to first object
toLastObject() - Moves position of iterator to last object
endIterator() - Moves position of iterator to final position (next will return nil from there)
hasNext()
countNext()
hasPrevious()
countPrevious()

Made changes to reflect new syntax in Beta3 release
Made changes to flatten() and initial() as they didn't work as intended for me.

…) and intial() as I could not get them to work as is. Pulled swift file out of playground which I used to test lazy evaluation. I was unable to get the tests to work on my mac so it's very well these issues are related to my beta.
@ALJCepeda
Copy link
Author

I was unable to do create or perform any tests on my beta. I've done all my tests in playground and committed so you can see a demo of my laziness.

@ALJCepeda ALJCepeda changed the title Lazy evaluation of chained methods Laziness in action! Jul 8, 2014
@ALJCepeda
Copy link
Author

Edit: Added tests to project

@ankurp
Copy link
Owner

ankurp commented Jul 8, 2014

Thanks for the Pull Request. Code looks good but had few comments.

Let's remove the playground file and just have examples in test and README.

Also was there a reason to change T[] to [T]?

Will check the compile issue as it was in working condition before.

@ALJCepeda
Copy link
Author

As I said in my second comment, I could not get the tests working on my beta. The rare times when it DID compile, it was riddled with errors. Maybe due to the changes in beta3? I chose to implement my tests as a demo in playground in the meantime.

Changing T[] to [T] is to reflect the syntax changes made in beta3. Also for for i in..myArray now must be for i in...myArray as .. is now reserved for half range specifiers.

@ankurp
Copy link
Owner

ankurp commented Jul 9, 2014

I see what you mean. I downloaded Beta 3 and fixed the compile error and warnings and merged that. #73

You can pull from master again so only your lazy eval changes show up in this PR. Lets remove the playground files for now from this PR as the test not being able to run is a Xcode Beta 3 issue. I will report this bug to Apple so its fixed by Beta 4.

@ALJCepeda
Copy link
Author

I only see syntax changes made to the latest commit. If you run the tests you'll see many of them fail, and crash with errors such as BAD_ACCESS and SIGABRT. In my latest commits I've commented out the tests that fail/crash. It was about of the tests if I remember.

@ALJCepeda
Copy link
Author

After parsing the tests I've been able to implement my own tests. Indexing in Xcode is very slow, so anybody opening the project for the first time will probably have to wait a while before it'll compile, but my current commit has all the offending code commented out. Playground's been deleted as well, so this should be good to complete the pull request I hope.

@ankurp
Copy link
Owner

ankurp commented Jul 10, 2014

There are some playground files I still see in the code to be merged.

For me compiling takes a long time for Tests but I get no error. Can you pull all of the code from master one more time if you haven't to make sure your changes are in sync.

I have noticed compile errors if you just use Xcode project so maybe compile using the Xcode workspace.

@ankurp
Copy link
Owner

ankurp commented Jul 10, 2014

After you remove those I can pull your changes on my machine and confirm that everything is good before merging.

Merged branches and commented out code that was preventing tests from
passing.
@ALJCepeda
Copy link
Author

Ok I think that should do it. More tests were able to pass but not all of them.

@ALJCepeda
Copy link
Author

So it turns out the issue I'm having has to do with AnyObjects and Generics. Put it simply, having chained methods that store AnyObject doesn't play nice with complex Swift Types. I changed the class to use generics exclusively, which solved that issue but some methods like flatten() still remain dysfunctional i think because of it's reliance on NSArrays to get the job done.

Changing generics also broke the iterator, and it'll have to be redone. All my tests involved in chaining was done through an iterator so...none of my tests will work now :(. Still this commit allows you to chain complex swift types, which I think is more desirable than an iterator at the moment.

@ALJCepeda
Copy link
Author

Also compiling and running tests is faster now.

@@ -0,0 +1,1259 @@
//
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file

@ankurp
Copy link
Owner

ankurp commented Jul 12, 2014

@ALJCepeda Thanks for contributing but it seems you are trying to do a lot of things in this pull requests. I would suggest we only make changes to add an array where we push the lazy eval closure function and add iterator methods. Lets not try to change a lot of things as it is leading to existing tests to fails and we do not know the exact cause.

Also you are changing the way these helper methods get called where type needs to be specified. For example, Dollar<Any>.first([1, 2, 3]) This is just a lot of unnecessary typing and type should be inferred.

@ankurp
Copy link
Owner

ankurp commented Jul 12, 2014

The compiler is also very buggy that I am having hard time trying to run the test as swift and SourceKit processes are using up 100% CPU and hogging all of the memory.

@ALJCepeda
Copy link
Author

You're right I'm trying too many things at once and I've made a mess of this pull request. I apologize. I've gone ahead and taken a step back and just added lazy evaluation of chained methods. I've deleted out all the commented code, but have left the tests that won't work on my system commented (such as flatten())

I had to take chaining methods out and put it into its own class called Bank. The reason I did this was because I could not get the Dollar to work with with complex Swift types. It's very possible there's something I'm misunderstanding so if that's the case then I apologize again. but using Generics instead of AnyObject was the only way to get it to work.

Please let me know what you think

@ALJCepeda
Copy link
Author

If you think this is something you can use, I'll merge and commit with your Ok.

@@ -15,7 +15,7 @@ extension Int {
///
/// :param callback The function to invoke that accepts the index
func times(callback: (Int) -> ()) {
(0..<self).eachWithIndex { callback($0) }
(0..self).eachWithIndex { callback($0) }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes an error in Beta 3. You will need to use ... or ..< notation

@ankurp
Copy link
Owner

ankurp commented Jul 13, 2014

I implemented the changes in this PR #76

@ALJCepeda ALJCepeda closed this Aug 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants