-
Notifications
You must be signed in to change notification settings - Fork 90
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
MINIFICPP-359 Generate connection name field if none provided, thereb… #231
Conversation
* is optional and defaults to 'id' | ||
* @return the parsed or generated UUID string | ||
*/ | ||
std::string getOrGenerateId(YAML::Node *yamlNode, const std::string &idField = "id"); | ||
std::string getOrGenerateField(YAML::Node *yamlNode, const std::string &field = "id"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the end goal of this PR? It seems that you want to generalize the function name to eventually be used elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
End goal is outlined in https://issues.apache.org/jira/browse/MINIFICPP-359. getOrGenerateField is used in this PR for name now, not just id: std::string name = getOrGenerateField(&connectionNode, "name");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you added the assignment, but I don't see how we are generating fields other than ID fields with that function. In this case it is okay because name will exist at this point in the code by virtue of the checkRequiredFields call.
In the case where someone else uses it, there is little to indicate other than const std::string &field = "id" that they will get a uuid field value per the conditional in getOrGenerateField. In my opinion they are getting or generating an ID in the current function, hence my confusion....and why I was wondering if there was intent to make this more general by providing some way of setting a default value other than a generated ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkRequiredField call was removed:
- checkRequiredField(&connectionNode, "name",
- CONFIG_YAML_CONNECTIONS_KEY);
So the current behavior is that name can be left unspecified. In that case it will be generated.
ae72ef5
to
4d49034
Compare
@phrocker removed getOrGenerateField change. |
…y enabling anonymous connections.
4d49034
to
f2bca0d
Compare
@phrocker fixed conflct should be ready for re-review. |
…y enabling anonymous connections. This closes apache#231. Signed-off-by: Marc Parisi <phrocker@apache.org>
…y enabling anonymous connections.
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 MINIFI-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:
For documentation related changes:
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.