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

#141 Implement updating comment instead of deleting and recreating #142

Closed
wants to merge 13 commits into from

Conversation

bmaehr
Copy link

@bmaehr bmaehr commented Mar 22, 2020

No description provided.

Copy link
Owner

@mc1arke mc1arke left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. The intent of what your PR title says sounds like a great feature, but there seems to be additional changes in your PR that are doing unrelated things, including commits from other Pull Requests.

Please squash the commits you need to make this feature work, and drop any unrelated commits (submit them under a separate PR if you want those features!). Please also ensure your code is correctly indented to make it easier to read.

@bmaehr
Copy link
Author

bmaehr commented Apr 20, 2020

Yeah, there are things from the other pull request and a big refactoring. I will first make the other pull requests be accepted, then they are automatically eliminated here.
I'm not going to undo the refactoring and include this feature into the existing spaghetti code.

@@ -107,6 +119,27 @@ public BranchConfiguration load(Map<String, String> localSettings, ProjectBranch
return new DefaultBranchConfiguration();
}

private Map<String, String> autoConfigure(Map<String, String> localSettings) {

Choose a reason for hiding this comment

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

I'm really looking forward to seeing this in the next release!

if (fileCommentEnabled) {
Map<String, String> headers = getHeaders();
final String pullRequestId = analysis.getBranchName();
final String mergeRequestDiscussionURL = getMergeRequestDiskussionsURL(pullRequestId);

Choose a reason for hiding this comment

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

This is the german spelling, it should use a c instead of a k. There are more variables and Methodnames with this typo.

@matthiasbalke
Copy link

@bmaehr good work! Looking forward to these features!

@mc1arke
Copy link
Owner

mc1arke commented Jun 13, 2020

@bmaehr are you still interested in progressing this feature?

@mc1arke mc1arke closed this Mar 6, 2021
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.

4 participants