Skip to content
This repository has been archived by the owner on Dec 5, 2022. It is now read-only.

added TimeInterval to func monkeyAround() #43

Merged
merged 5 commits into from
Oct 12, 2017
Merged

added TimeInterval to func monkeyAround() #43

merged 5 commits into from
Oct 12, 2017

Conversation

graemerycyk
Copy link

This doesn’t remove the infinite running of monkeyAround() but simply adds the ability to run swiftMonkey tests based on a specific amount of time.

Thanks to @mohamede1945 for raising the issue and @DwayneCoussement for answering my question about this improvement 🙌

This doesn’t remove the infinite running of monkeyAround() but simply adds the ability to run swiftMonkey tests based on a specific amount of time.

Thanks to mohamede1945 for raising this issue and  dwaynecoussement for answering my question about this improvement 🙌
Copy link
Member

@wojciechczerski wojciechczerski left a comment

Choose a reason for hiding this comment

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

Hello @grycyk and thank you very much for this PR! There is one small thing I would like you to change. When this is done we can do a release with this new feature 😄

public func monkeyAround() {
while true {
/// Generate random events or fixed-interval events based on a specific duration or infinitely
public func monkeyAround(forDuration duration: TimeInterval) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please make it an additional method so that we keep the parameterless monkeyAround() still available? I can see from the commit summary that:

This doesn’t remove the infinite running of monkeyAround() ...

But I'm afraid it does. When you try to compile the tests for the provided sample app the compilation will fail.

Copy link
Author

Choose a reason for hiding this comment

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

True true, .infinitely can be used but that will mean they will have to update their test(s), sorry, I didn't consider that 🙈 okay, I will make a fix later today and will update the PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I totally forgot about the .infinity value! With that the solution is even more simple:

public func monkeyAround(forDuration duration: TimeInterval = .infinity) {
    ...
}

Great idea @grycyk 👍

@graemerycyk
Copy link
Author

@wojciechczerski it should be fixed now i think. 👍

p.s a typo in my commit message "separate method" 😅

@@ -154,6 +154,15 @@ public class Monkey {
actRegularly()
}
}

/// Same as monkeyAround() but allows you to set a specific duration of time for the test
Copy link
Member

Choose a reason for hiding this comment

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

Since we changed the signature we could also provide a more accurate Quick Help documentation, for example:

/// Generate random events or fixed-interval events based on a specific duration or infinitely.
///
/// - Parameter duration: The duration for which to generate the random events.

@wojciechczerski
Copy link
Member

I made one more small request to the documentation but apart from that it should be good.

BTW: I am fine if you do a force push to your branch - this might be helpful that you don't have to populate the commits for every change request (like created it as a spearate method). But it's up to you of course ;)

@graemerycyk
Copy link
Author

So will we keep it in one method?

Adding public func monkeyAround(forDuration duration: TimeInterval = .infinity) I will add the extra documentation comment too, will push it over in about an hour.

@wojciechczerski
Copy link
Member

I would prefer to see it just as you designed it but with the default .infinity duration.

With such API someone who looks at the code will immediately understand that this method will monkey around for some time and by default it runs forever. And the best is that you get all this information just from the method signature without even looking at the documentation. On the other hand in order to understand what does the monkeyAround() do, you need to read the docs.

@graemerycyk
Copy link
Author

super!! ill fix the PR asap 👍

Graeme Rycyk added 2 commits October 12, 2017 12:22
This was the my initial idea to improve monkeyAround() with some added tweaks such as further expalnation in the comment plus the default value of .infinity to the TimeInterval.
@graemerycyk
Copy link
Author

@wojciechczerski I think this should be the final change needed. 👍

///
/// - Parameter duration: The duration for which to generate the random events.
///
/// - TimeInterval: Set to .infinity by default.
Copy link
Member

Choose a reason for hiding this comment

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

The parameter name is duration so it should be:

/// - duration: Set to .infinity by default.

Improvement: You can surround .infinity with the `` code markers to make it look even better in the Quick Help:

/// - duration: Set to `.infinity` by default.

screen shot 2017-10-12 at 13 47 38

Suggestion

How about we put the Set to .infinity by default. in the main description of the duration parameter? It would look like this:

/// - Parameter duration: The duration for which to generate the random events.
///                       Set to `.infinity` by default.

screen shot 2017-10-12 at 13 48 59

What do you think @grycyk ?

@graemerycyk
Copy link
Author

Done! 👍 @wojciechczerski I have never really used documentation from the comment like that with Quick Help before so thanks for the pointers! 🙌

@wojciechczerski wojciechczerski merged commit f9f3afa into zalando:master Oct 12, 2017
@wojciechczerski
Copy link
Member

Aaaand it's merged as well! Thank you very much for your contribution @grycyk ! I hope I wasn't too annoying with my picky comments 😃

I'll try to make a release today in the evening.

@graemerycyk
Copy link
Author

@wojciechczerski Not at all, if anything is worth doing, its worth doing right! 💯
I will look at helping out again, I think this is a interesting project! 🙌

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants