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.

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

feat: Use binding object to check for Binder#hasChanges #17861

Merged
merged 14 commits into from
Oct 20, 2023

Conversation

benedikt-roth
Copy link
Contributor

@benedikt-roth benedikt-roth commented Oct 19, 2023

Description

Allows to check whether specific binding has been changed. So far there is only a generic Binder#hasChanges, which checks all bindings. The new method allows to check whether a specific binding has changes.

Fixes #17395

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

Bindings. Overloads the existing Binder#hasChanges
method.

GH issue vaadin#17395
@CLAassistant
Copy link

CLAassistant commented Oct 19, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mcollovati mcollovati added the Contribution PRs coming from the community or external to the team label Oct 19, 2023
@mcollovati
Copy link
Collaborator

@benedikt-roth do you mind signing the CLA and formatting the source code as suggested in #17861 (comment)?

@github-actions
Copy link

github-actions bot commented Oct 19, 2023

Test Results

1 014 files  +   539  1 014 suites  +539   1h 14m 46s ⏱️ + 37m 30s
6 507 tests +5 753  6 466 ✔️ +5 742  41 💤 +11  0 ±0 
6 762 runs  +5 781  6 714 ✔️ +5 769  48 💤 +12  0 ±0 

Results for commit 3dc1e66. ± Comparison against base commit c77d34f.

♻️ This comment has been updated with latest results.

@benedikt-roth
Copy link
Contributor Author

@benedikt-roth do you mind signing the CLA and formatting the source code as suggested in #17861 (comment)?

Sure, done that and also fixed the formatting.

Copy link
Collaborator

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, the change looks good.
I would add tests for hasChanges when the binding has been removed from the binder.

I also wonder if we could add the hasChanges() method also on the Binding interface, so that it would be possible to simply do a binding.hasChanges().
The implementation in BindingImpl could use the internal binder field to check it this binding is in the changeBindings set.
And it will throw and IllegalStateException if the binder is null.

However, this is not mandatory for the PR to be merged

@benedikt-roth
Copy link
Contributor Author

Thanks for the contribution, the change looks good. I would add tests for hasChanges when the binding has been removed from the binder.

I also wonder if we could add the hasChanges() method also on the Binding interface, so that it would be possible to simply do a binding.hasChanges(). The implementation in BindingImpl could use the internal binder field to check it this binding is in the changeBindings set. And it will throw and IllegalStateException if the binder is null.

However, this is not mandatory for the PR to be merged

Let me look into this one. Definitely would be useful. If it is a quick change, let's include this in this PR.

Copy link
Collaborator

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

The change looks good to me. Let's see if the validation passes.
Good job @benedikt-roth

@vaadin-bot vaadin-bot added +0.1.0 and removed +0.0.1 labels Oct 20, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mcollovati mcollovati merged commit 39fa6d9 into vaadin:main Oct 20, 2023
28 checks passed
@benedikt-roth benedikt-roth deleted the feat/has-changes-for-binding branch October 21, 2023 08:35
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.3.0.alpha1 and is also targeting the upcoming stable 24.3.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Add a method to Binder to check if a specific Binding changed
4 participants