-
Notifications
You must be signed in to change notification settings - Fork 78
refactor(macro)!: change to builder pattern #372
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
Conversation
c00a928
to
8bc33fb
Compare
Decided to go with builder pattern and move the outer macro back in priority. Would still like to have it, but there are some cases that really need a builder, so this is the foundation. |
4253a65
to
f46ca0a
Compare
So this is a chonker... Most of it is taken from #174 which already had some review. I tried to rebase onto current master, but it was to old to get that done cleanly. I pulled what I could and integrated it with the new stuff and added stubs back in. |
migrate outdated builder to current version BREAKING CHANGE: The old macros were dependent on execution order and have been causing trouble with language servers. They are replaced by a builder. See the migration guide at https://davidcole1340.github.io/ext-php-rs/migration-guides/v0.14.html for information on how to migrate. Fixes: #99, #131, #327 Refs: #174, #335 Co-authored-by: David Cole <[email protected]> Co-authored-by: Pierre Tondereau <[email protected]> Co-authored-by: Daniil Gentili <[email protected]>
f46ca0a
to
aeed538
Compare
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.
Make some comment about API and doc, like i have said in the original PR i think this is the best for the future.
Did not look at the implementation, not that much time, but IMO if the API is good implementation can always be fixed later
aeed538
to
6db05a4
Compare
91a1ae8
to
8060a79
Compare
Other solutions require unstable features
8060a79
to
b6724f0
Compare
I'll just merge this now. Has been open long enough for comments. Lets just wait a bit longer before the next release to find issues. |
.parse_meta() | ||
.map_err(|_| anyhow!("Unable to parse attribute."))?; | ||
fn get_method_props<'a>(self) -> ::std::collections::HashMap<&'static str, ::ext_php_rs::props::Property<'a, #path>> { | ||
todo!() |
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.
It looks like this was never fixed, so actually getters / setters don't work at all right now
BREAKING CHANGE: The old macros were dependent on execution order and have been causing trouble with language servers. They are replaced by a builder. See the migration guide at https://davidcole1340.github.io/ext-php-rs/migration-guides/v0.14.html for information on how to migrate.
Fixes: #99, #131, #327
Refs: #174, #335