-
Notifications
You must be signed in to change notification settings - Fork 178
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
Documentation Change Request: Best Practices #418
Comments
Thanks for the feedback, this is very constructive. I'll try and address your 4 points below: Prefer nested context classes over inherited classesUnderstand what you are saying, I'll strengthen the recommendation to use caution when nesting or inheriting. Use of behaviorsYep, totally agree with you. We don't use behaviors at all in my workplace where we can help it, and I think they should be deprecated in an upcoming major release. This is an oversight on my part, I'll address the documentation to discourage their use. I'd be interested to hear any ideas you had around a substitute that helps reduce repeated Because to be a single statement100% agree, I'll add that in. Put lambda expressions on new lineI agree there is an element of preference here, so I'm happy to soften the language and simply recommend the format, since it aligns with (most) of the tests in the MSpec repo. However it's my opinion that the advice stands because:
|
I'll close this for now, as I'll assume the above addresses your issue. |
I have a few recommendations for changes to the Best Practices documentation. Overall I agree with the spirit of what is currently reflected, but in a few places I feel like it may actually serve to encourage bad practices rather than good ones.
My first recommendation concerns the item: "Prefer nested context classes over inherited classes". While I don't necessarily disagree with the statement in and of itself, I would submit that both techniques are to be avoided. So yes, nested contexts would be preferable to inheritance, but I would submit that we should prefer repeating shared context over either the use of nested contexts or inherited classes. I do agree with the warning that too much use of nested contexts can be confusing (i.e. leads to test obscurity), but I don't think the warning goes far enough to discourage the practice.
The second recommendation concerns the absence of any recommendations against the use of behaviors. For the same reasons as the first recommendation, use of behaviors should be discouraged. As I'm sure you are aware, Aaron Jensen's opinion was that this feature should never have been included. The fact that the document calls out best practices around naming Behaviors kind of legitimizes their use. It would be best to have an explicit recommendation to not use behaviors and perhaps call out that the are maintained solely for backward compatibility.
The third recommendation concerns the absence of a recommendation for Because to be a single statement. Context/Specification is about performing a single action for an established context, so it's arguably more important for the Because to be a single statement than for the It in terms of adherence to the spirit of context/specification (although I agree that both should be a single line).
The forth recommendation concerns the item: "Put lambda expressions on new line". This feels more like a personal preference that sneaked in as a "best practice" rather than being reflective of some general consensus within the mspec user community. For expressions that have no need to wrap, I would argue that it's actually more readable to keep both Because, It, and Cleanup expressions on the same line. In the event that the statements are lengthy, you'd certainly want to place them on a separate line, but I feel like the guidance for both is more of a general coding recommendation rather than anything specific to Mspec.
The text was updated successfully, but these errors were encountered: