Large commit warning test. Fixes #362

[Fastjur]

Issue #362

[coveralls]

Coverage increased (+0.1%) to 57.066% when pulling dcf25e92cc1e870e628fa506cf084eb37d654ac6 on LargeCommitWarningTest into 3bf90d0fb86b9b1a6f27ecf73371bb28ecd44d7f on master.

[coveralls]

Coverage increased (+0.1%) to 57.066% when pulling 5d7cb935a665f6fab52ec8acf089b888bb2e4278 on LargeCommitWarningTest into 3bf90d0fb86b9b1a6f27ecf73371bb28ecd44d7f on master.

[jwgmeligmeyling]

Nice work! :+1: Not all my comments have to be addressed, but please have a look at using some actual objects here and there, as it should make the test less verbose, and more open to extension of the warning generator.

What is your general thought on the testability of the LargeCommitWarning? And do you find it a sensible warning?

[Fastjur]

Addressing your previous questions:

What is your general thought on the testability of the LargeCommitWarning?
Testability was OK, had some trouble setting up the diffModel though, mainly due to my own incompetence. It was not hard to setup the actual test cases though.

And do you find it a sensible warning?
This really depends on the situation, it is, however, convention to keep your commits small, as this improves readability and the general structure of the repo will be less messy. So yes, I do think this is a sensible warning

[jwgmeligmeyling]

Testability was OK, had some trouble setting up the diffModel though, mainly due to my own incompetence. It was not hard to setup the actual test cases though.

The DiffModel is indeed hard to set up. I think this also comes because its structure relies quite heavily on the use of generics, in order to use mostly the same structure for DiffBlameModels (which are DiffModels with authorship information added per line).

This really depends on the situation, it is, however, convention to keep your commits small, as this improves readability and the general structure of the repo will be less messy. So yes, I do think this is a sensible warning.

Yes, there will be a lot of false positives. Thats also one reason why the max size is configurable. I am however not sure what the currently configured threshold is 👼

[Fastjur]

Yes, there will be a lot of false positives. Thats also one reason why the max size is configurable. I am however not sure what the currently configured threshold is :angel:

False positives are actually okay. Haven't had them occur yet, and committing something with 500+ lines should feel a bit off anyway. The current threshold is 10 files and 500 lines by the way, seems reasonable to me.

[coveralls]

Coverage increased (+0.1%) to 57.087% when pulling b93752e56a05708f02e54992479f2d42c8a98877 on LargeCommitWarningTest into 3bf90d0fb86b9b1a6f27ecf73371bb28ecd44d7f on master.

[jwgmeligmeyling]

@Fastjur could you perhaps bring this branch up to date with master?

[Fastjur]

@JWGmeligMeyling Yep sorry, it is up to date now.

[jwgmeligmeyling]

Thanks! 👍

[coveralls]

Coverage increased (+0.04%) to 56.952% when pulling 349326e613277cb37405b4822359dca348ae2747 on LargeCommitWarningTest into 7a67b5b0b4e25102cd95fe49514d6f85db9249af on master.