[18.0][FIX] hr_holidays_natural_period: Create no_validation leave - #254
[18.0][FIX] hr_holidays_natural_period: Create no_validation leave#254SirPyTech wants to merge 1 commit into
Conversation
|
Hi @victoralmau, |
OCA-git-bot
added
mod:hr_holidays_natural_period
|
This PR has the |
pedrobaeza
changed the title
| self.assertEqual(leave_type.leave_validation_type, "no_validation") | ||
| for manager_group in manager_groups: | ||
| self.assertNotIn(manager_group, self.env.user.groups_id) | ||
|
|
There was a problem hiding this comment.
Please don't add empty lines inside a method.
There was a problem hiding this comment.
Hi, thanks for having a look!
Sometimes I use blank lines in a method to keep logical sections of the method separated, this kind of usage is even mentioned in PEP8 https://peps.python.org/pep-0008/#blank-lines:
Use blank lines in functions, sparingly, to indicate logical sections.
Is there any specific reason why they should be removed? If that's just an opinion-based style decision I'd rather keep them.
There was a problem hiding this comment.
Yes, because:
sparinglymeans infrequent, not the usual thing to do.- keep the current style of the module, which no lines inside the method.
- What is for you a logical section? It can be different from me, and adding a blank line doesn't enforce what that "logical section" does. If you really want to have logical sections, add a line with a comment explaining what the next logical section is.
There was a problem hiding this comment.
I see that https://github.com/OCA/hr-holidays/pull/254/changes#r3361035074 triggered more heated comments on the rest of the code 🔥 sorry to have upset you.
Just to give you more context, this test method follows the AAA testing pattern (see for instance https://dev.to/coderjay06/the-three-a-s-of-unit-testing-b22) and most of the times I find it useful to give a clear structure to test methods:
Arrange the state of the data to set it up for testing.
Act on the data through some method that performs an action.
Assert that the result from acting on that data is what we expect it to be.
The 3 logical sections in this method follow this structure.
| # Act | ||
| leave = self._create_hr_leave(leave_type, "3000-01-01", "3000-01-01") | ||
|
|
||
| # Assert |
There was a problem hiding this comment.
This comment is redundant, as the Python code already infers that you assert in the next line.
There was a problem hiding this comment.
This clarifies that we are in the third step of the AAA testing pattern (see for instance https://dev.to/coderjay06/the-three-a-s-of-unit-testing-b22).
| for manager_group in manager_groups: | ||
| self.assertNotIn(manager_group, self.env.user.groups_id) | ||
|
|
||
| # Act |
There was a problem hiding this comment.
This clarifies that we are in the second step of the AAA testing pattern (see for instance https://dev.to/coderjay06/the-three-a-s-of-unit-testing-b22).
SirPyTech
left a comment
SirPyTech
left a comment
There was a problem hiding this comment.
Apparently we have different opinions on using blank lines, but that happens, I hope this won't block the PR 🤞.
| self.assertEqual(leave_type.leave_validation_type, "no_validation") | ||
| for manager_group in manager_groups: | ||
| self.assertNotIn(manager_group, self.env.user.groups_id) | ||
|
|
There was a problem hiding this comment.
I see that https://github.com/OCA/hr-holidays/pull/254/changes#r3361035074 triggered more heated comments on the rest of the code 🔥 sorry to have upset you.
Just to give you more context, this test method follows the AAA testing pattern (see for instance https://dev.to/coderjay06/the-three-a-s-of-unit-testing-b22) and most of the times I find it useful to give a clear structure to test methods:
Arrange the state of the data to set it up for testing.
Act on the data through some method that performs an action.
Assert that the result from acting on that data is what we expect it to be.
The 3 logical sections in this method follow this structure.
| for manager_group in manager_groups: | ||
| self.assertNotIn(manager_group, self.env.user.groups_id) | ||
|
|
||
| # Act |
There was a problem hiding this comment.
This clarifies that we are in the second step of the AAA testing pattern (see for instance https://dev.to/coderjay06/the-three-a-s-of-unit-testing-b22).
| # Act | ||
| leave = self._create_hr_leave(leave_type, "3000-01-01", "3000-01-01") | ||
|
|
||
| # Assert |
There was a problem hiding this comment.
This clarifies that we are in the third step of the AAA testing pattern (see for instance https://dev.to/coderjay06/the-three-a-s-of-unit-testing-b22).
|
My thumb rule is simple: less is more. To not have to do more vertical scrolling. All of this pattern thing is very good, but that's a methodology, not something that should be literally ported putting that comments. You don't even put The most important thing here is that this not a new module, just an added test, where the rest of the tests are not done that way, so please respect the current general style. |
5 participants
Forward port of #248.
There is no change to the module's code because apparently this version is not affected by the same issue that was present in
16.0.