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

Allow having bindings referencing const Properties #72

Merged
merged 5 commits into from
Jul 2, 2024

Conversation

phyBrackets
Copy link
Contributor

Initial work for issue #29

Copy link
Contributor

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

Good change, do we really need the ConstPropertyNode class?

src/kdbindings/node.h Outdated Show resolved Hide resolved
@LeonMatthesKDAB
Copy link
Contributor

LeonMatthesKDAB commented Jun 4, 2024

@phyBrackets Can you please rebase this and your other PRs.

CI should then pass again :)

Edit: Nevermind, I added the option to do it through Github, so can do it myself :)

@LeonMatthesKDAB
Copy link
Contributor

Hm, enabling clazy seems to have generated some new warnings through this PR. Good thing that's actually now failing CI 🤔

Please fix them before this PR can be merged.

@phyBrackets
Copy link
Contributor Author

Hm, enabling clazy seems to have generated some new warnings through this PR. Good thing that's actually now failing CI 🤔

Please fix them before this PR can be merged.

So that is easy to fix, but we still want to see if we want the ConstPropertyNode or not :)

@LeonMatthesKDAB
Copy link
Contributor

So that is easy to fix, but we still want to see if we want the ConstPropertyNode or not :)

I definitely want to be able to reference const Properties, I just wonder why we need an entirely new Node type for this?

Could you try modifying the existing PropertyNode type to accept const references? Or is there any reason it really has to be its own class? At the moment it just seems like a lot of code duplication...

@LeonMatthesKDAB LeonMatthesKDAB force-pushed the const_property_node branch 2 times, most recently from 7994473 to 7ab22cf Compare June 14, 2024 07:30
Copy link
Contributor

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

This already looks pretty good, but I think we can get away without the IsConst conditionals.

src/kdbindings/node.h Outdated Show resolved Hide resolved
src/kdbindings/node.h Outdated Show resolved Hide resolved
@phyBrackets phyBrackets force-pushed the const_property_node branch from d3904fe to e80936e Compare June 30, 2024 13:57
@phyBrackets phyBrackets force-pushed the const_property_node branch from e80936e to a74f98c Compare June 30, 2024 14:08
@LeonMatthesKDAB LeonMatthesKDAB merged commit dcb3831 into main Jul 2, 2024
5 checks passed
@LeonMatthesKDAB LeonMatthesKDAB deleted the const_property_node branch August 28, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants