adding patch property to Commit class #1416
Conversation
dduraipandian
commented
Feb 26, 2022
dduraipandian
commented
- added patch property to Commit class
- added relevant test cases and tested it.
| @@ -149,6 +149,81 @@ def check_entries(d): | |||
| self.assertEqual(commit.committer_tz_offset, 14400, commit.committer_tz_offset) | |||
| self.assertEqual(commit.message, "initial project\n") | |||
|
|
|||
| def test_patch_with_parent(self): | |||
There was a problem hiding this comment.
Since this is a patch with parent, I think it should be easy to find a patch with is much smaller. If there truly is no way, the expected value could be read from a file.
There was a problem hiding this comment.
Thanks for reviewing it. I will try to get commit with a smaller patch.
| """ | ||
| commit = self.rorepo.commit('c0740570b31f0f0fe499bf4fc5abbf89feb1757d') | ||
| patch = commit.patch | ||
| self.assertEqual(patch[:200], expected_path_result[:200]) |
There was a problem hiding this comment.
I don't understand why it would slice the result into lines like this. This also means it would possibly only test for equivalence on a subset of lines, missing some lines behind the 600 mark, for example.
There was a problem hiding this comment.
I can't test with whole patch text. I have to set maxDiff = None to disable limit and test. For test_with_parent, I will try to get a commit with smaller patch. For test_with_no_parent, which is first commit, I will save patch text to file and set maxDiff in setup and test it.
There was a problem hiding this comment.
Then I think it would be important to make clear how maxDiff works in the property's documentation so callers know how to deal with it. To me for instance it wasn't clear this exists.
Or said differently, if there is a technical limitation there is less of a need to work around it from my side, as long as it can be made clear in the docs how to handle it.
| +# W504""" | ||
| commit = self.rorepo.commit('33ebe7acec14b25c5f84f35a664803fcab2f7781') | ||
| patch = commit.patch | ||
| self.assertEqual(patch[:200], expected_path_result[:200]) |
There was a problem hiding this comment.
As above, I don't think the slice syntax should be necessary here. If it is, a description on why that is would be necessary, it could be passed as third argument to assertEqual I believe.
There was a problem hiding this comment.
have to set maxDiff = None to disable limit and test. I will do the change and test it.
| @@ -315,6 +315,18 @@ def stats(self) -> Stats: | |||
| text = self.repo.git.diff(self.parents[0].hexsha, self.hexsha, '--', numstat=True) | |||
| return Stats._list_from_string(self.repo, text) | |||
|
|
|||
| @property | |||
| def patch(self) -> str: | |||
There was a problem hiding this comment.
It probably would be worth describing that this method can fail if the processed bytes aren't in the encoding that python expects (as it returns str and not bytes). Hence this method can throw an exception and from my experience it's more than likely that this happens to some.
This also means that at some point in the future and if there is demand one could provide a variant of this method that returns bytes instead.
There was a problem hiding this comment.
Thanks for this note. I will check to see if i can return bytes than string and leave the choice to users to convert them to string. Is that okay?
There was a problem hiding this comment.
That would be okay, yes. If it doesn't work with returning bytes right now then the possibility of decoding errors could be highlighted in the documentation specifically.
|
Closed due to inactivity. |
