Skip to content
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.

Sign up for GitHub

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

[IMP] make _set_lines_issue working on empty issue #190

Merged
merged 2 commits into from
Jul 3, 2022
Merged

[IMP] make _set_lines_issue working on empty issue #190

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e707c6
[IMP] make _set_lines_issue working on empty issue
legalsylvain Jul 1, 2022
4ba3d91
[REF] refactor to improve _set_lines_issue coverage
legalsylvain Jul 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/oca_github_bot/tasks/migration_issue_bot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ def _set_lines_issue(gh_pr, issue, module):
lines.append(new_line)
added = True
lines.append(line)

# make the addition working on an empty migration issue
if not added:
lines.append(new_line)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A typical migration issue ends with comments after the check list of modules:

image

So this would add a line after the comment which may not look very nice.

I wish we had better tests for _set_lines_issue.

Copy link
Collaborator Author

@legalsylvain legalsylvain Jul 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A typical migration issue ends with comments after the check list of modules:

that's not the point of the PR.

The line will be added only if the issue doesn't contain module list.
In your screenshot, my patch has no effect, because previous code will add the line in the correct place, and so added will be true.

That is just a trivial patch to initialize migration issue, if there is no module list.

I wish we had better tests for _set_lines_issue.

Well. Yes it's possible. The simplier is to change the function _set_lines_issue(gh_pr, issue, module) by _set_lines_issue(gh_pr_number, gh_pr_login, body_issue, module)``
So simple and precise test can be written, covering all the cases. (without pointing on existing PR as this is the case for the time being https://github.com/OCA/oca-github-bot/blob/master/tests/test_migration_issue_bot.py#L37).
with the current design it is not possible to test the result of the function, because the result is not predictive.

but I have 3 pending PR around _set_lines_issue so I can do this refactor, but if it's possible, after having merged The current PR.

return lines


Expand Down