PR-to-issue fixes for story points and priority#477
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@webbnh I made the changes as per suggestion can you please review it again.. |
|
/retest |
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
The refactor turned out very nicely, thanks!
I've flagged a couple of things which I think you should consider, but I'm approving anyway.
| return None | ||
| token = config["sync2jira"].get("github_token") | ||
| headers = {"Authorization": "token " + token} if token else {} | ||
| github_client = Github(token, retry=5) | ||
| headers, github_client = u_issue._github_client(config) |
There was a problem hiding this comment.
Given that we have cross-module references, github_client() should not be named with an underscore prefix!
There was a problem hiding this comment.
changed the name to get_github_client ad if we remove underscore there will be conflict with local variable.
| self.assertEqual(mock_reformat.call_count, expected_count) | ||
| self.assertEqual(mock_add_project.call_count, expected_count) | ||
| self.assertEqual(mock_pr_from_github.call_count, expected_count) |
There was a problem hiding this comment.
I'm inclined to think that these are implementation details which we don't really want to require -- whether these functions are called directly or a specific number of times is not part of the interface contract.
If you really want to exercise this, the assertions should be checking what was returned, not how it was instantiated.
Otherwise, we're locking ourselves into a particular implementation, which is counterproductive.
There was a problem hiding this comment.
replaced the three call-count assertions with a single assertion on the returned values — verifying that github_prs() yields exactly the PR objects produced by PR.from_github.
2 participants
Problem:
PRs are syncing as issues, but the story points and priority fields are not being synced.
Fix:
In the upstream layer, we already fetch GitHub project values such as
estimate(used as story points) andpriorityfor issues. The same logic has now been extended for PRs as well, ensuring that both story points and priority are synced correctly.