-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fixes for several LGTM alerts #38033
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
Conversation
…cts/g/elastic/elasticsearch/snapshot/dist-1916470085-1548143539391/files/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java#x4e06dc48702dac54:1) * [Non-final method invocation in constructor](https://lgtm.com/projects/g/elastic/elasticsearch/snapshot/dist-1916470085-1548143539391/files/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java?sort=name&dir=ASC&mode=heatmap#xaeb362a26c0fdaf1:1) * [Non-final method invocation in constructor](https://lgtm.com/projects/g/elastic/elasticsearch/snapshot/dist-1916470085-1548143539391/files/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java?sort=name&dir=ASC&mode=heatmap#x5c00888239d02f3a:1)
|
My github profile should now list my secondary email. Sorry about that oversight. |
|
@krisds Would you mind splitting these changes into multiple PRs, one for each file you have changed? That way we can assess the changes in different areas of the code separately which will make it easier to review and get any accepted changes merged. With these kinds of changes its often difficult to review as one PR as for each change there may be subtle impacts on functionality that we need to consider (not saying they are not bugs, just that we have to consider if the change will impact something else in that area). Splitting the changes into separate PRs makes it easier for different people who are familiar with different parts of the codebase to review and merge each change separately Thanks |
|
No problem. I will do that. |
|
I'm going to close this since we are tracking these fixes in the newly opened separate PRs. Thanks for separating the PRs @krisds |
Hi. I'm an LGTM developer, and had some time to fix a few alerts flagged in Elasticsearch by our analyses.
There should be one commit per file, and the commit messages have links back to the original alerts to make it easy for you to check what each thing is about.
Let me know if this is helpful or not, what I can do better, or if there are any specific (types of) alerts you're interested in which I can look at in the future.