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

glib: Implement Regex #947

Merged
merged 1 commit into from
Oct 27, 2023
Merged

glib: Implement Regex #947

merged 1 commit into from
Oct 27, 2023

Conversation

davidmhewitt
Copy link
Contributor

@davidmhewitt davidmhewitt commented Feb 3, 2023 •
edited
Loading

I'm interested in generating bindings for https://github.com/elementary/granite which exposes GLib.Regex as part of its public API, so it would be good to have these in the glib crate to depend on.

@sdroege sdroege self-requested a review February 3, 2023 12:14
@sdroege sdroege added this to the 0.17.0 milestone Feb 3, 2023
sdroege
sdroege reviewed Feb 3, 2023
View reviewed changes
glib/src/regex.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Feb 3, 2023
View reviewed changes
glib/src/auto/regex.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Feb 3, 2023
View reviewed changes
glib/src/auto/regex.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Feb 3, 2023
View reviewed changes
glib/src/auto/match_info.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Feb 3, 2023
View reviewed changes
Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise, thanks!

@jf2048
Copy link
Member

jf2048 commented Feb 4, 2023 •
edited
Loading

Not a blocker, but it would be good if eventually we could have a documentation note here saying this should not really be used by Rust programs except for compatibility reasons. No one should ever be writing new code with this unless necessary, e.g. calling those libgranite APIs that need it. There are much better native Rust implementations, even the pcre2 crate is better than using this.

Edit: some more background here https://gitlab.gnome.org/GNOME/glib/-/issues/1085#note_1270415

@sdroege sdroege modified the milestones: 0.17.0, 0.18.0 Feb 8, 2023
@davidmhewitt davidmhewitt requested a review from sdroege February 10, 2023 10:34
@bilelmoussaoui
Copy link
Member

Would you mind rebasing this one?

bilelmoussaoui
bilelmoussaoui reviewed Oct 26, 2023
View reviewed changes
Copy link
Member

@bilelmoussaoui bilelmoussaoui left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

glib/Gir.toml Show resolved Hide resolved
glib/src/auto/regex.rs Show resolved Hide resolved
sdroege
sdroege reviewed Oct 27, 2023
View reviewed changes
glib/src/regex.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Oct 27, 2023
View reviewed changes
glib/src/regex.rs Outdated Show resolved Hide resolved
@sdroege
Copy link
Member

sdroege commented Oct 27, 2023

Looks good to me apart from those trivial things. Once that's all covered, can you squash the commits together?

bilelmoussaoui
bilelmoussaoui reviewed Oct 27, 2023
View reviewed changes
glib/src/lib.rs Outdated Show resolved Hide resolved
glib/src/lib.rs Outdated Show resolved Hide resolved
@davidmhewitt
Copy link
Contributor Author

Thank you both for the feedback, I'll work on addressing the last few comments over the next few hours.

But first, a question:

It looks like I initially manually implemented a lot of these methods so we could use impl IntoGStr in the arguments instead of &str. The methods generated by gir are otherwise identical.

Is it still the case that it would be preferred to have manually written bindings just to get impl IntoGStr here, or should I revert most of these methods back to the automatically generated versions that use &str?

@sdroege
Copy link
Member

sdroege commented Oct 27, 2023

Keep them like this until gir supports generating them like this. That's what the comment in Gir.toml should say why they're not autogenerated.

@davidmhewitt
glib: Implement Regex
ac892d2 
These modules are inefficient and should not be used by Rust programs except for
compatibility with GLib.Regex based APIs.

All methods are implemented except for g_regex_replace_eval
@davidmhewitt
Copy link
Contributor Author

I have resolved all feedback apart from the non-blocking comment about the missing implementation of g_regex_replace_eval and squashed my commits.

Thanks again for your reviews, let me know if there's anything else I can do here.

@bilelmoussaoui bilelmoussaoui added the needs-backport PR needs backporting to the current stable branch label Oct 27, 2023
@bilelmoussaoui
Copy link
Member

Looks good enough, we can always iterate later if needed. Thank you for your contribution

@bilelmoussaoui bilelmoussaoui merged commit 03bce03 into gtk-rs:master Oct 27, 2023
48 checks passed
@sdroege
Copy link
Member

sdroege commented Nov 12, 2023

This has some soundness issues, see #1223

@bilelmoussaoui bilelmoussaoui removed the needs-backport PR needs backporting to the current stable branch label Feb 6, 2024
@renovate renovate bot mentioned this pull request Nov 13, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdroege sdroege sdroege left review comments

@bilelmoussaoui bilelmoussaoui bilelmoussaoui approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@davidmhewitt @jf2048 @bilelmoussaoui @sdroege