You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/04/24 10:43:34 UTC
[GitHub] [nifi-minifi-cpp] hunyadi-dev opened a new pull request #770: Minificpp 1203 - upgrade linter version to 1.4.4
hunyadi-dev opened a new pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
arpadboda commented on pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#issuecomment-632126610
Merged in 29460fa1a035096b251a7f2a323ddffcec2c4210
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#discussion_r428173660
##########
File path: libminifi/src/utils/OsUtils.cpp
##########
@@ -24,16 +24,18 @@
#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN
#endif
-#include <Windows.h>
#include <sddl.h>
+#include <Windows.h>
Review comment:
Judging from the appveyor output, the order of these includes matter. Not sure of the exact reason, but in general windows headers are a mess, so I'm not surprised. :(
See the end of the log here: https://ci.appveyor.com/project/ApacheSoftwareFoundation/nifi-minifi-cpp/builds/33004452
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#discussion_r428014274
##########
File path: libminifi/src/io/StreamFactory.cpp
##########
@@ -16,12 +16,13 @@
* limitations under the License.
*/
#include "io/StreamFactory.h"
+
#include <atomic>
#include <memory>
#include <mutex>
#include <string>
-#include <utils/GeneralUtils.h>
+#include <utils/GeneralUtils.h> // NOLINT
Review comment:
why do we need NOLINT here?
Also, end of line comments should have 2 preceding spaces.
##########
File path: libminifi/test/archive-tests/FocusArchiveTests.cpp
##########
@@ -16,24 +16,23 @@
* limitations under the License.
*/
+#include <algorithm>
#include <iostream>
#include <map>
+#include <memory>
#include <set>
#include <string>
-#include <algorithm>
-#include <memory>
#include <utility>
-#include "../TestBase.h"
+#include <archive.h> // NOLINT
+#include <archive_entry.h> // NOLINT
Review comment:
why do we need NOLINT here?
End of line comments should have 2 spaces before and 1 after the `//`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on pull request #770: Minificpp 1203 - upgrade linter version to 1.4.4
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#issuecomment-618940064
:exclamation: This PR depends on #769: ensuring that the `make linter` builds are clean with both versions of the linter.
Originally the linter was sourced from the google style-guide fork which is not very well maintained, this is now replaced with the script shipped by the cpplint/cpplint repository maintainers
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#discussion_r428174378
##########
File path: libminifi/test/tensorflow-tests/TensorFlowTests.cpp
##########
@@ -19,14 +19,14 @@
#include <memory>
#include <string>
-#include <tensorflow/cc/framework/scope.h>
-#include <tensorflow/cc/ops/standard_ops.h>
-#include <processors/PutFile.h>
-#include <processors/GetFile.h>
-#include <processors/LogAttribute.h>
+#include <processors/GetFile.h> // NOLINT
+#include <processors/LogAttribute.h> // NOLINT
+#include <processors/PutFile.h> // NOLINT
+#include <tensorflow/cc/framework/scope.h> // NOLINT
+#include <tensorflow/cc/ops/standard_ops.h> // NOLINT
Review comment:
Regarding my previous suggestion: I suggest trying to convert our headers to use quotation marks and leave 3rd parties as is, even if it means that we need to use NOLINT.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#discussion_r428080271
##########
File path: libminifi/src/io/StreamFactory.cpp
##########
@@ -16,12 +16,13 @@
* limitations under the License.
*/
#include "io/StreamFactory.h"
+
#include <atomic>
#include <memory>
#include <mutex>
#include <string>
-#include <utils/GeneralUtils.h>
+#include <utils/GeneralUtils.h> // NOLINT
Review comment:
This is incorrectly recognized as a C header. Also `NOLINT` comments do not need to be preceded by 2 spaces.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#discussion_r428571885
##########
File path: libminifi/test/tensorflow-tests/TensorFlowTests.cpp
##########
@@ -19,14 +19,14 @@
#include <memory>
#include <string>
-#include <tensorflow/cc/framework/scope.h>
-#include <tensorflow/cc/ops/standard_ops.h>
-#include <processors/PutFile.h>
-#include <processors/GetFile.h>
-#include <processors/LogAttribute.h>
+#include <processors/GetFile.h> // NOLINT
+#include <processors/LogAttribute.h> // NOLINT
+#include <processors/PutFile.h> // NOLINT
+#include <tensorflow/cc/framework/scope.h> // NOLINT
+#include <tensorflow/cc/ops/standard_ops.h> // NOLINT
Review comment:
My initial idea was to do it here, but you're right, it's better not to mix in this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#issuecomment-631574636
> I find the Google include ordering rules strange, but that's in CONTRIB.md, so I guess we're better off enforcing them.
>
> Thanks for the upgrade!
Agreed on the rules being strange, I would much rather prefer the llvm standard too. I could patch the linter for changing the order, but that would mean that we would need to apply the same patch every time we want to upgrade the linter version (and that everyone developping a code with linter assistance would need to apply the same patch to their own linter). I don't think the change is worth the effort.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#discussion_r428167985
##########
File path: libminifi/src/io/StreamFactory.cpp
##########
@@ -16,12 +16,13 @@
* limitations under the License.
*/
#include "io/StreamFactory.h"
+
#include <atomic>
#include <memory>
#include <mutex>
#include <string>
-#include <utils/GeneralUtils.h>
+#include <utils/GeneralUtils.h> // NOLINT
Review comment:
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
I think if we replaced the angle brackets with quotation marks, the warning would go away. Could you try this? If it doesn't work, them I'm fine with leaving the NOLINT comments of such cases in place.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#discussion_r428081193
##########
File path: libminifi/test/archive-tests/FocusArchiveTests.cpp
##########
@@ -16,24 +16,23 @@
* limitations under the License.
*/
+#include <algorithm>
#include <iostream>
#include <map>
+#include <memory>
#include <set>
#include <string>
-#include <algorithm>
-#include <memory>
#include <utility>
-#include "../TestBase.h"
+#include <archive.h> // NOLINT
+#include <archive_entry.h> // NOLINT
Review comment:
This is incorrectly recognized as a C header. Also `NOLINT` comments do not need to be preceded by 2 spaces.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#discussion_r428501369
##########
File path: libminifi/test/tensorflow-tests/TensorFlowTests.cpp
##########
@@ -19,14 +19,14 @@
#include <memory>
#include <string>
-#include <tensorflow/cc/framework/scope.h>
-#include <tensorflow/cc/ops/standard_ops.h>
-#include <processors/PutFile.h>
-#include <processors/GetFile.h>
-#include <processors/LogAttribute.h>
+#include <processors/GetFile.h> // NOLINT
+#include <processors/LogAttribute.h> // NOLINT
+#include <processors/PutFile.h> // NOLINT
+#include <tensorflow/cc/framework/scope.h> // NOLINT
+#include <tensorflow/cc/ops/standard_ops.h> // NOLINT
Review comment:
Will do that in a separate PR, right? I don't want to rebase my linter PR again.
##########
File path: libminifi/test/tensorflow-tests/TensorFlowTests.cpp
##########
@@ -19,14 +19,14 @@
#include <memory>
#include <string>
-#include <tensorflow/cc/framework/scope.h>
-#include <tensorflow/cc/ops/standard_ops.h>
-#include <processors/PutFile.h>
-#include <processors/GetFile.h>
-#include <processors/LogAttribute.h>
+#include <processors/GetFile.h> // NOLINT
+#include <processors/LogAttribute.h> // NOLINT
+#include <processors/PutFile.h> // NOLINT
+#include <tensorflow/cc/framework/scope.h> // NOLINT
+#include <tensorflow/cc/ops/standard_ops.h> // NOLINT
Review comment:
Will do that in a separate PR, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #770: MINIFICPP-1203 - upgrade linter version to 1.4.4 and fix relevant linter errors
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #770:
URL: https://github.com/apache/nifi-minifi-cpp/pull/770#discussion_r428555849
##########
File path: libminifi/src/utils/OsUtils.cpp
##########
@@ -24,16 +24,18 @@
#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN
#endif
-#include <Windows.h>
#include <sddl.h>
+#include <Windows.h>
Review comment:
Corrected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org