Add IniBuilder #442
Add IniBuilder #442
Conversation
Xenira
left a comment
Xenira
left a comment
There was a problem hiding this comment.
Haven't had a detailed look yet. But looks good.
I think we can keep it without a feature guard for now.
|
Would you mind adding documentation in the And thanks again for contributing! |
|
Yep, I can add a guide for that. Where do you think it should go? |
Qard
force-pushed
the
ini-builder
branch
6 times, most recently
from
c43e8b6 to
7862694
Compare
Qard
force-pushed
the
ini-builder
branch
2 times, most recently
from
08634cb to
6e2c008
Compare
|
Seems the ini builder API didn't exist until 8.2. I've added compile gates to |
Guess you could pass that in here: https://github.com/davidcole1340/ext-php-rs/blob/cafaecc692c474281925716f256f0842e4580b28/build.rs#L177 See windows build: https://github.com/davidcole1340/ext-php-rs/blob/cafaecc692c474281925716f256f0842e4580b28/windows_build.rs#L53 |
Qard
force-pushed
the
ini-builder
branch
3 times, most recently
from
af9e073 to
9d8436f
Compare
|
I did some restructuring of the build file to include an ApiVersion enum which can be used for managing both the cfg flag names and C define names in the stacked form it was using before, so we can do |
Qard
force-pushed
the
ini-builder
branch
9 times, most recently
from
7a0495c to
addaeb1
Compare
|
@Qard merged the clippy PR, you can rebase |
|
Thanks! Rebased now. 🙂 |
|
I still seem to be getting yelled at by clippy for unrelated things. 🤔 The issues appear to be fixed by the clippy allows in #456, so maybe I'll just wait for that to land too? 😅 |
|
Also needs a rebase and regeneration of the |
There was a problem hiding this comment.
I think we also miss the opportunity to add an integration test by supporting setting ini_entries in SapiBuilder.
We could have in addition something like this:
#[test]
fn test_sapi() {
let mut ini = IniBuilder::new();
ini.define("foo=bar").unwrap();
ini.quoted("memory_limit", "128M").unwrap();
let mut builder = SapiBuilder::new("test", "Test");
builder = builder.ini_entries(ini.finish());
// ...
|
I've updated to include all the feedback except moving the |
Qard
force-pushed
the
ini-builder
branch
4 times, most recently
from
d92c838 to
d64d6bf
Compare
Qard
force-pushed
the
ini-builder
branch
4 times, most recently
from
c8d8838 to
e9cda5e
Compare
Xenira
left a comment
Xenira
left a comment
There was a problem hiding this comment.
I've updated to include all the feedback except moving the
CStringScope. Just need to figure out where to put that. 😅
While I am usually against util folders I guess its better than having it in the src root, as that is getting a little full by now. Can't think of anything better right now, so I'd put int into src/util.
Can always move it later if we can think of anything better :)
Other than that this looks really good. Thank you ❤️
Qard
force-pushed
the
ini-builder
branch
2 times, most recently
from
7ac2371 to
03e84b3
Compare
|
I moved that to |
4 participants
This might only be useful in combination with the
embedfeature so it may make sense to gate this behind that feature, but it doesn't depend on it in any way--it can exist without it. I left the feature-gating out, but I could change that if you prefer.