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/11/16 18:34:22 UTC
[GitHub] [nifi-minifi-cpp] szaszm opened a new pull request #938: MINIFICPP-1408 thrown objects should be derived from std::exception
szaszm opened a new pull request #938:
URL: https://github.com/apache/nifi-minifi-cpp/pull/938
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:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] 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)?
- [x] If applicable, have you updated the LICENSE file?
- [x] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [x] 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 GitHub Actions CI results 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 closed pull request #938: MINIFICPP-1408 thrown objects should be derived from std::exception
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #938:
URL: https://github.com/apache/nifi-minifi-cpp/pull/938
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #938: MINIFICPP-1408 thrown objects should be derived from std::exception
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #938:
URL: https://github.com/apache/nifi-minifi-cpp/pull/938#discussion_r525108396
##########
File path: extensions/sftp/client/SFTPClient.h
##########
@@ -49,6 +50,12 @@ enum class SFTPError : uint8_t {
SFTP_ERROR_UNEXPECTED
};
+struct SFTPException : Exception {
+ explicit SFTPException(const SFTPError err)
+ :Exception{ExceptionType::FILE_OPERATION_EXCEPTION, fmt::format("SFTP Error {0}", static_cast<int>(err))}
Review comment:
this is just something to think about, not a change request
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #938: MINIFICPP-1408 thrown objects should be derived from std::exception
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #938:
URL: https://github.com/apache/nifi-minifi-cpp/pull/938#discussion_r525899629
##########
File path: extensions/sftp/client/SFTPClient.h
##########
@@ -49,6 +50,12 @@ enum class SFTPError : uint8_t {
SFTP_ERROR_UNEXPECTED
};
+struct SFTPException : Exception {
+ explicit SFTPException(const SFTPError err)
+ :Exception{ExceptionType::FILE_OPERATION_EXCEPTION, fmt::format("SFTP Error {0}", static_cast<int>(err))}
Review comment:
yes, the implicit convertibility is indeed troublesome (and the extra scope specifier we save when accessing an instance might not be worth it), although in this specific case I would have gone with changing the parameter type from `const SFTPError&` to `SFTPError::Type`
----------------------------------------------------------------
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 #938: MINIFICPP-1408 thrown objects should be derived from std::exception
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #938:
URL: https://github.com/apache/nifi-minifi-cpp/pull/938#discussion_r525391415
##########
File path: extensions/sftp/client/SFTPClient.h
##########
@@ -49,6 +50,12 @@ enum class SFTPError : uint8_t {
SFTP_ERROR_UNEXPECTED
};
+struct SFTPException : Exception {
+ explicit SFTPException(const SFTPError err)
+ :Exception{ExceptionType::FILE_OPERATION_EXCEPTION, fmt::format("SFTP Error {0}", static_cast<int>(err))}
Review comment:
I have some negative feedback regarding the conversion to SMART_ENUM: the enum values are implicitly convertible to integer which makes it less safe than `enum class`. This resulted in compiling but invalid code when the wrong overload of `LastSFTPError::operator=` was called and I had to introduce differently named functions.
I'm leaning towards keeping SMART_ENUM in this case but I'll be more careful about introducing it in new places, weighing the convenient string conversion against the safety tradeoff in each case.
----------------------------------------------------------------
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 #938: MINIFICPP-1408 thrown objects should be derived from std::exception
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #938:
URL: https://github.com/apache/nifi-minifi-cpp/pull/938#discussion_r525109454
##########
File path: extensions/sftp/client/SFTPClient.h
##########
@@ -49,6 +50,12 @@ enum class SFTPError : uint8_t {
SFTP_ERROR_UNEXPECTED
};
+struct SFTPException : Exception {
+ explicit SFTPException(const SFTPError err)
+ :Exception{ExceptionType::FILE_OPERATION_EXCEPTION, fmt::format("SFTP Error {0}", static_cast<int>(err))}
Review comment:
I thought about it but then went for the easier solution. I might change it after I figure out the reason for the test failures.
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #938: MINIFICPP-1408 thrown objects should be derived from std::exception
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #938:
URL: https://github.com/apache/nifi-minifi-cpp/pull/938#discussion_r525899629
##########
File path: extensions/sftp/client/SFTPClient.h
##########
@@ -49,6 +50,12 @@ enum class SFTPError : uint8_t {
SFTP_ERROR_UNEXPECTED
};
+struct SFTPException : Exception {
+ explicit SFTPException(const SFTPError err)
+ :Exception{ExceptionType::FILE_OPERATION_EXCEPTION, fmt::format("SFTP Error {0}", static_cast<int>(err))}
Review comment:
yes, the implicit convertibility is indeed troublesome (and the extra scope specifier we save when accessing an instance might not be worth it), although in this specific case I would have went with changing the parameter type from `const SFTPError&` to `SFTPError::Type`
----------------------------------------------------------------
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] adamdebreceni commented on a change in pull request #938: MINIFICPP-1408 thrown objects should be derived from std::exception
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #938:
URL: https://github.com/apache/nifi-minifi-cpp/pull/938#discussion_r525108234
##########
File path: extensions/sftp/client/SFTPClient.h
##########
@@ -49,6 +50,12 @@ enum class SFTPError : uint8_t {
SFTP_ERROR_UNEXPECTED
};
+struct SFTPException : Exception {
+ explicit SFTPException(const SFTPError err)
+ :Exception{ExceptionType::FILE_OPERATION_EXCEPTION, fmt::format("SFTP Error {0}", static_cast<int>(err))}
Review comment:
I am not saying that we should change it in this PR (or in any PR for that matter), but turning `SFTPError` into a `SMART_ENUM` could benefit us, (e.g. here we could then write `err.toString()`)
----------------------------------------------------------------
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