Clean up RuboCop offences.

[lcreid]

We are using the approach described in: https://tech.offgrid-electric.com/rubocop-getting-to-green-in-a-huge-rails-app-12d1ad6678eb to implement a more consistent coding style in this project. RuboCop has been added to the Travis CI step, so offences cause the build to fail.

We've created a .rubocop_todo.yml file to disable a number of cops that we haven't had time to fix yet.

We'd be happy to receive your pull request (PR) to fix the code and remove the cop from .rubocop_todo.yml. This would be a good issue to take on if you want to practice your refactoring skills, or if you haven't ever submitted a PR, and you want to learn how to do a PR. Of course, you can also submit a PR just to make our code more consistent.

Here's a suggested approach:

1. Take a look at .rubocop_todo.yml and choose an offence that you want to work on. .rubocop_todo.yml includes a count of how many offences need to be fixed. You can choose how ambitious you want to be. If you're new to the process, you might be better off with an offence that has few occurrences.
2. Add a comment to this issue indicating which offence you're working on, so others don't duplicate your work.
3. Remove the offence from .rubocop_todo.yml.
4. Run rake rubocop to find the offences.
5. Fix them.
6. Run rake to make sure all the tests still pass, and that you've fixed the offences.
7. Push your changes and submit a pull request.

If you're not familiar with the Contributing document, you should read it before starting the above.

[simmerz]

@lcreid a lot of these could be cleaned up by moving each field type into its own module file, and moving various things to private methods within those. I'd suggest a lib/bootstrap_form/fields directory, within which each field type can have its own separate file. This will also make the code easier to read.

Happy to tackle this if you think it's an appropriate direction.

[lcreid]

It would be great if you could take this on! The only constraint we have is not to break anyone's existing code.

To that end, I would suggest not changing the namespace of any of the method calls documented in the README. If that means the lib/bootstrap_form directory gets full, let's live with that for now. However, feel free to push back on that if you have a solution for which you can show that it won't break other people's code.

Why don't you do one method and submit a PR for it? Pick an easy one so it's not too much effort, in case we decide to go a different direction. I'm excited to see what you'll come up with.

Please excuse the bureaucracy, but would you mind opening a separate issue for this refactoring? It would be helpful to be able to track the discussion of refactoring in its own issue. You can put a reference in the new issue back to this one so we don't lose the connection.

One final note: Beware that some of the complexity in the code is there because the problem is complex. Be prepared for test cases breaking in unexpected ways. :-) And if you have any doubt, don't be shy about adding test cases. The test suite runs quickly now, so we can afford to err on the side of too much testing.

Thanks again!

[simmerz]

@lcreid I've created #517 for this. Happy to work on it. Good idea too re presenting the direction.

Top-line, I don't believe there is any need to break compatibility with the existing code. What I'm proposing is effectively a a concern pattern. Including each of the inputs to make them more maintainable without actually changing too much of the structure. I'll see how that pans out as I go.

[simmerz]

Working on Metrics/AbcSize, Metrics/CyclomaticComplexity, Rails/FilePath, Rails/OutputSafety and Style/GuardClause, Layout/IndentHeredoc, Metrics/LineLength, Metrics/ParameterLists, Naming/PredicateName, Style/IfUnlessModifier and Metrics/MethodLength. Submitted #522 to fix those

[lcreid]

Fixed by #518 and #522.