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.

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

[FIX] ocabot migration duplicating lines in migration issue? #215

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

SirTakobi
Copy link
Contributor

Fix #214

@SirTakobi SirTakobi marked this pull request as ready for review October 10, 2022 10:24
@@ -39,7 +39,7 @@ def _set_lines_issue(gh_pr_user_login, gh_pr_number, issue_body, module):
if added: # Bypass the checks for faster completion
lines.append(line)
continue
groups = re.match(rf"^- \[( |x)\] {module}( |\r)", line)
groups = re.match(rf"^- \[( |x)\] {module}", line)
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct at all, as it can match with partial module names. Why is it incorrect? Maybe we can match the end of the line instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review!
I haven't managed to reproduce exactly the issue #214 because I don't know how the Migration issue is created, but I think this regex might be the cause.

I have found that \b is made exactly for matching full words, see https://docs.python.org/3/library/re.html#regular-expression-syntax, can you check if it looks better now?

A new line was created because the regex was expecting something after the module name
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Thank you for tackling the problem!

@pedrobaeza
Copy link
Member

@sbidoul can you merge and deploy, please?

@sbidoul sbidoul merged commit 74bee31 into OCA:master Oct 18, 2022
@sbidoul
Copy link
Member

sbidoul commented Oct 18, 2022

Thank you @SirTakobi. I just deployed.

@pedrobaeza
Copy link
Member

Let's see if we finally end with this problem. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ocabot migration duplicating lines in migration issue?
3 participants