fix(sprint_cleaner): fixes default arguments of the sprint cleaner

[ankitkataria]

• earlier named arguments were used, moved to using default arguments to
  prevent ArgumentError
• made the passing of the third argument in sprint_cleaner_spec verbose to make sense of its purpose

Fixes #197

[cornelius]

Could you please also add a test which exposes the bug?

[Ana06]

as @cornelius says, a test case would be nice 😉

[Ana06]

what about

expect(subject.cleanup('7Zar7bNm', '72tOJsGS', true).to be

instead of an extra variable? 🤔

[ankitkataria]

Actually, cornelius had earlier suggested that the extra verbosity, because he thought that the purpose of that third parameter wasn't evident. So he suggested moving to a named parameter.

[ankitkataria]

@Ana06 the expection arose due to using named parameters. when no value for the last parameter was passed it threw ArgumentError. Now that've moved to default params, that error won't be occuring anymore. I'm not sure how I'm supposed to write a test for that. 🤔

[cornelius]

@ankitkataria The extra verbosity I suggested was not for the test. With a positional parameter you indeed should just call it without an extra variable. My point was about the signature of the function so that when you call it you are forced to provide a named parameter instead of a boolean. When you read the code where the function is called it's then obvious what it does. When there is only a false or true as a parameter you need to look up the definition of the function to understand what the calling code does. That's why for booleans named parameters are generally preferrable.

Regarding the test, what would be ideal is, if you write a test which fails with the old version of the code. That would expose the bug that the code didn't work anymore when no parameter was given. This test should go into the tests of the Cli class because that's where the issue occurred. When you then apply a fix the test will be green and it will be guaranteed that it won't occur again, because there is a test now. That's he gist of test-driven development.

As for the fix I think you should fix it in the Cli class when calling the method, not going back to positional parameters.

Hope that clear up what I was thinking :-)

[ankitkataria]

Thank you for the really elaborate explanation @cornelius 😄! I fixed it in cli.rb I added a test in sprint_cleaner_spec and cli_spec to see about the ArgumentErrorss that are being raised.

[cornelius]

This looks much better.

There still is an issue with the tests. You check for not raising an ArgumentError. The test works because it doesn't raise this error but it raises an error from WebMock. This is because in the cli_spec.rb the methods called in the cleanup method are not mocked so they are trying to do an HTTP access. Any other error raised would also not make the tests fail. This might hide actual issues.

It would be better to check that no error is raised and make sure that the test environment is set up in a way that this doesn't happen. We have the helper methods to mock a full board. Maybe they can be used?

[ankitkataria]

@cornelius added mocking of the planning and target boards. 😉 Also fixed a minor typo in cli.rb in the cleanup_sprint method

[ankitkataria]

Hi! @cornelius @Ana06 😄! what all is left to be done in this PR?

[agraul]

@ankitkataria there was a change to the cli interface. The code is moved to lib/cli/scrum.rb and during the process, your additional cli flag was lost 😭. I've added a new flag to that command and fixed the setup issues in #204, maybe you could use that as a base and add yours again?

[Ana06]

@ankitkataria do you need help to do the changes? if so, please let us know 😉

[ankitkataria]

Hey @Ana06, this pr had been open for a while and it looks like a lot of code was changed after that. I'm closing this pr and opening a new one with the required flag #208. I hope it does the job 😄 .