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

[#1459] do not merge in case .rultor file is in PR #1865

Merged
merged 5 commits into from
Feb 8, 2024

Conversation

pnatashap
Copy link
Contributor

  • logic is added
  • test is disabled, as MkPull doesn't allow to work with files (too many code to write without it)

@pnatashap
Copy link
Contributor Author

@yegor256 please take a look

} else
if (pull.allChecksSuccessful()) {
new Answer(comment).post(
true,
Copy link
Owner

Choose a reason for hiding this comment

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

@pnatashap indentation here is definitely wrong (not 4 spaces), but I can't understand why Qulice is not complaining about this...

@@ -36,6 +36,8 @@ QnMerge.checks-are-failed=Can't merge it. Some CI checks were failed. \
Apparently, the pull request is not ready to be \
merged since it has some problems. \
Please, fix them first.
QnMerge.system-files-affected=Can't merge it. File `.rultor.yml` is affected by \
this pull request.
Copy link
Owner

Choose a reason for hiding this comment

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

@pnatashap would be great if we can explain to the user, why we don't accept modifications to this particular file. Let's say: "Can't merge this pull requests, because the file .rultor.yml was modified. This is a security threat. Make a separate pull request with the modification of this particular file and ask project architect to merge it manually."

Copy link
Owner

@yegor256 yegor256 Feb 8, 2024

Choose a reason for hiding this comment

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

@pnatashap also, I think it's better not to hardcode the name of the file here, but use %s and pass the name of it as a parameter to String.format

Matchers.containsString(
QnMergeTest.PHRASES.getString(
"QnMerge.system-files-affected"
)
Copy link
Owner

Choose a reason for hiding this comment

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

@pnatashap indentations here are also unusual, but Qulice doesn't complain... very strange.

@yegor256
Copy link
Owner

yegor256 commented Feb 8, 2024

@pnatashap many thanks for the contribution! see my comments above

@pnatashap
Copy link
Contributor Author

@yegor256 please take a look.
qulice does not complains most probably because the version here is 0.13, while the last one is 0.22 and there are 1000+ changes required. It is nice to reformat it, because some of the requirements are not compatible - in this author and version is required and it is prohibited now. May be also good to have public IntelliJ settings file, because these indentations are just default.

@yegor256
Copy link
Owner

yegor256 commented Feb 8, 2024

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 8, 2024

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@pnatashap pnatashap requested a review from yegor256 February 8, 2024 13:47
@rultor rultor merged commit a5e6c7d into yegor256:master Feb 8, 2024
5 checks passed
@rultor
Copy link
Collaborator

rultor commented Feb 8, 2024

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 9min)

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.

3 participants