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
fixed Rultor login prefix not recognized correctly in comments, when … #1000
Conversation
…delimited by comma, by using regex for yegor256#821
@original-brownbear Many thanks for the PR, let me find a reviewer for it |
@mkordas review this pull request plz |
@original-brownbear I'm on it |
Logger.info(this, "mention found in #%d", comment.issue().number()); | ||
req = Req.DONE; | ||
final Matcher matcher = Pattern.compile( | ||
String.format(".*\\b?(%s\\b).*", prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@original-brownbear here you have some complex regexp, but I see just one test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not done here yet sorry my mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkordas now this should be handled too.
@original-brownbear please see a few comments above |
…e mention being preceeded by a comma for yegor256#821
@mkordas I think I addressed all points now. Made it match like this now:
|
@original-brownbear I'm on it again |
Logger.info(this, "mention found in #%d", comment.issue().number()); | ||
req = Req.DONE; | ||
final Matcher matcher = Pattern.compile( | ||
String.format("(?:^|(?:.*?(?:\\s|,)))(%s)\\b.*?", prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@original-brownbear right now this pattern is even more complex - can you extract it to constant and add a Javadoc with explanation to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkordas done, hope it's not too verbose.
* Matches the login when at the beginning of the comment string or when | ||
* preceded by a space or comma. | ||
* Login has to be bound by a word boundary to the right. | ||
* Only captures the @ sign and login. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@original-brownbear looks great for me, thanks a lot!
@original-brownbear good job |
@rultor merge |
@rultor try to merge |
@original-brownbear @yegor256 Oops, I failed. You can see the full log here (spent 16min)
|
@rultor merge again |
@original-brownbear @yegor256 Oops, I failed. You can see the full log here (spent 12min)
|
@rultor merge |
@original-brownbear @yegor256 Oops, I failed. You can see the full log here (spent 8min)
|
@yegor256 please take care of this PR as well |
@rultor merge |
@original-brownbear can you close this PR? |
@rultor deploy |
@mkordas jup all done here : ) |
@mkordas since quality is good, I just added 10 mins to @original-brownbear (our architect) in transaction AP-37D02265W46925311 |
@original-brownbear are you new a architect here? :) |
@mkordas yes :) |
@original-brownbear congrats! I think the biggest challenge would be to improve stability of Rultor in production. |
@mkordas thanks :), 100% agreed:
I already have experience building integration tests with much more complicated docker setups and made tickets for the dependencies, this should be very doable :) |
…delimited by comma, by using regex for #821