Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
CEP27: toolkit Snippet #297
CEP27: toolkit Snippet #297
Changes from 18 commits
0a96da5
9ea0df2
6686d60
1c75d2b
a5bc8d8
3fcb4a7
2f09ba2
2e79678
3afaa92
26b7515
4bb1528
d045749
5d09ded
5770f22
0507887
d895250
49c25ba
7c4c0c4
2b066c2
fdfa326
03f4ca9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If you remove only this line (not exposed in input), then the snippet can be used in both modes, and developer chooses name of member variable.
In fact, that might be better, because otherwise developer has to know name of member variable defined in the snippet.
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.
this is correct, (and I like the versatility it provides)
I guess it is a balance between versatility and amount of work on the developer side:
if we only support without-inheritance everything should be includable in the snippet.
if we want to support both, developers have to add:
#include 'toolkit/my_toolkit.h
#include
toolkit/snippet.cycpp.h` in the class core.(this is also why without inheritance has my (slight) preference.)
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 good thing with removing the variable it would allow both with and without inheritance
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.
Even without inheritance, I'm not sure we want to assume that the member variable is always defined by the snippet... what if you want two positions?
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.
sure but then what is the use of the snippet ? because some member variable are defined in the snippet (at least the one expose in the input.)
I don't see how to use the snippet with 2 positions.
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.
This might not be the best example of including more than one instance. In addition, there may be a need to initialize each instance which gets complicated again. Nevertheless, I think asking the developer to explicitly declare their member variable is not too much to ask.
We should open this up to additional discussion, e.g. on the mailing list.
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 still think there is a benefit to having a single snippet file and then letting user choose the variable name in the non-inheritance case. We can include that in the example in the documentation.
Once we get that resolved, and clean up this CEP, I think I'm ready to approve/merge this and the follow-up PR's in the code repos.