Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Extend bindings modeling #69

Merged
merged 50 commits into from
Dec 18, 2023
Merged

Extend bindings modeling #69

merged 50 commits into from
Dec 18, 2023

Conversation

rvermeulen
Copy link

This extends the number of supported bindings and introduces a binding parser to deal with binding strings.

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 15, 2023
View reviewed changes
javascript/frameworks/ui5/test/lib/Bindings/test.js Fixed Show fixed Hide fixed
javascript/frameworks/ui5/test/lib/Bindings/test.js
value: "{/root/name}"
});

var oInputWithLateBinding = this.byId("foo");

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused variable oInputWithLateBinding.
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 16, 2023
View reviewed changes
javascript/frameworks/ui5/test/lib/Bindings/test.js
return Controller.extend("foo", {
onInit: function() {
// Early property binding
var oInputWithEarlyPropertyBinding = new sap.m.Input({

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused variable oInputWithEarlyPropertyBinding.
javascript/frameworks/ui5/test/lib/Bindings/test.js

// Early dynamic property binding
const model = "model";
var oInputWithEarlyDynamicPropertyBinding = new sap.m.Input({

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused variable oInputWithEarlyDynamicPropertyBinding.
javascript/frameworks/ui5/test/lib/Bindings/test.js
oInput.bindProperty("value", "name");

// Early composite binding
var oInputWithEarlyContextBinding = new sap.m.Input({

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused variable oInputWithEarlyContextBinding.
javascript/frameworks/ui5/test/lib/Bindings/test.js
});

// Early property metadata binding
var oLabel = new sap.m.Label({

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused variable oLabel.
@rvermeulen
Add draft binding string parser
d0f9ac1
@rvermeulen
Add draft binding modeling
7e34a50
@rvermeulen
Add support for binding path parsing in path values
8fc7310
@rvermeulen
Add predicate to parse bindings
276664f
@rvermeulen
Use binding parser on binding strings
149d64a
@rvermeulen
Address incorrect identification of context binding attribute
34c0093
@rvermeulen
Reuse StringBinding class
b599ada
@rvermeulen
Add test for XML property and context binding
751981f
@rvermeulen
Add JavaScript binding calls to the list of considered string
d7d23d4
@rvermeulen
Generalize late JavaScript bindings
e108f54
@rvermeulen
Extend the binding test cases
e521396
@rvermeulen
Handle composite bindings
49e7d71
@rvermeulen
Add qldoc to clarify the different bindings
e37fb86
@rvermeulen
Format QL module
969f8a4
@rvermeulen
Improve early JavaScript binding detection
78f69fb
@rvermeulen
Add HTML test case to bindings tests
db28c78
@rvermeulen
Address incorrect expected file
f95dc01
@rvermeulen
Add support for JSON bindings
ee0dcda
@rvermeulen
Add qldoc to Binding class
0813a1e
@rvermeulen
Apply query format
c8b05ed
@rvermeulen
Rename StringBinding to BindingString
1fa88f2
@rvermeulen
Improve Bindings module interface by hiding implementation details
54945ae
@rvermeulen
Add JSON test file with bindings
d656503
@rvermeulen
Rename BindingValue to StaticBindingValue
979d14b
@rvermeulen
Generalize dynamic binding to Node instead
d7c5cfb
jeongsoolee09
jeongsoolee09 requested changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@jeongsoolee09 jeongsoolee09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another fantastic huge improvement and I learned a lot reading through this PR. Added more docstrings for documentation and to check if we are on the same page.

@rvermeulen rvermeulen marked this pull request as ready for review December 7, 2023 16:34
@rvermeulen
Remove superfluous signature
067f34c
@rvermeulen
Fix QL doc formatting
d2e1c1b
@rvermeulen
Replace late binding newtype with abstract class
a59d696
@rvermeulen
Change JSON property binding definition
1c82a52 
We substitute the value with the object because that combined with the
key represents which property of the object gets bounded.
The object represents a better binding target since we don't represents
Json object members and the value is the binding itself.
@rvermeulen
Add a class BindingTarget to represents binding targets
05e043e
@rvermeulen
Add member predicate to get the string representation of a binding path
22530c6
@rvermeulen
Add QLdocs
8248bfe
jeongsoolee09
jeongsoolee09 reviewed Dec 7, 2023
View reviewed changes
Copy link
Contributor

@jeongsoolee09 jeongsoolee09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one additional comment

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/Bindings.qll Show resolved Hide resolved
@jeongsoolee09
Copy link
Contributor

jeongsoolee09 commented Dec 18, 2023 •
edited
Loading

One more addition please: Since views can be declared in JS code as well (example), can you cover this as well? Maybe like:

private newtype TBindingTarget =
    ...
} or
TJsonPropertyBindingTarget(JsonObject target, string key, Binding binding) {
  binding = TJsonPropertyBinding(target, key, _)
} or
TJsViewBindingTarget(/* TBD */) {
   /* TBD */
}

@jeongsoolee09
Copy link
Contributor

No need to do this now; works great on my branch. TY!

@jeongsoolee09 jeongsoolee09 self-requested a review December 18, 2023 20:08
jeongsoolee09
jeongsoolee09 approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@jeongsoolee09 jeongsoolee09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid LGTM

@jeongsoolee09 jeongsoolee09 merged commit 627a9f6 into main Dec 18, 2023
@jeongsoolee09 jeongsoolee09 deleted the rvermeulen/bindings branch December 18, 2023 20:09
@jeongsoolee09 jeongsoolee09 mentioned this pull request Jan 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeongsoolee09 jeongsoolee09 jeongsoolee09 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rvermeulen @jeongsoolee09