-
Notifications
You must be signed in to change notification settings - Fork 3
Fix FN cap sources #109
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
Fix FN cap sources #109
Conversation
javascript/frameworks/cap/test/models/cds/remoteflowsources/remoteflowsource.cds.json
Outdated
Show resolved
Hide resolved
| * ``` | ||
| * parameters named `req` are captured in the above example. | ||
| */ | ||
| class ServiceinCDSHandlerParameter extends RemoteFlowSource { |
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.
Could this make use HandlerRegistration in CDS.qll ? Or is this exactly what you wanted to avoid?
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.
technically that part could work, but yeah,
then we would still need to modify the definition of UserDefinedApplicationService which only describes js defined extensions of extends cds.ApplicationService, which I was thinking was cleaner/fits nicer into the overall model, but maybe isnt worth the extra (not undoable but more involved) refactoring
mbaluda
left a comment
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.
LGTM
fixes #101
only adds to the
RemoteFlowSourcemodel, not the CDS modelling because that would have required a deeper (I think currently unnecessary) addition to the concept of service definition