Implement Debug for all types that work with QDebug #1162
Conversation
jnbooth
changed the title
jnbooth
changed the title
Debug for all types that work with QDebug
jnbooth
changed the title
Debug for all types that work with QDebug
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1162 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 71 71
Lines 11967 11967
=========================================
Hits 11967 11967 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi @jnbooth, thank you for this contribution!
It's great to see you're using CXX-Qt actively and I'm very happy to see the improvements that come out of it 😃
This PR mostly looks good. I believe it can be simplified a little bit more, as the toQString function should actually not be necessary anymore, and there are a few nitpicks to take care of.
Then we should be able to merge this 🥳
| @@ -33,6 +33,13 @@ drop(T& value) | |||
| template<typename T> | |||
| QString | |||
| toQString(const T& value) | |||
There was a problem hiding this comment.
This template should no longer be necessary.
If toString() is already a member function, we don't need to create a free function toQString, but can just access it directly via CXX:
i.e.:
// Maybe with #[doc(hidden)], depending on the situation
#[rust_name="to_qstring"]
fn toQString(self: &T);Please remove this function and replace the existing references with direct member access.
There was a problem hiding this comment.
The idea of this template is to allow types to have more flexibility in how they define to_qstring(). Some types have extra parameters for toString. For others, we might not want to have a to_qstring method at all (rather than relying on #[doc(hidden)]. Or we might decide not to have methods like that in general and use From<&Type> for QString, which is more idiomatic. So by using this template, types aren't "locked in" to a particular implementation; we can keep the Display logic separate from everything else.
That said, I of course defer to you. What should be the rubric for whether or not to use #[doc(hidden)]?
There was a problem hiding this comment.
Thank you for the explaining the motivation.
I see this PR as mostly about implementing Debug and Display, which is useful and I don't want to hold up with this discussion too much.
In my opinion, if there's a method that we can use via CXX directly, we should use it that way for two reasons:
- This is usually the easiest to maintain
- It sticks closely to the original Qt API, which is good for discoverability.
We already decided that we should use direct bindings whenever possible and deviate from the Qt API as little as possible, so let's stick with this here as well.
We only rename toString to to_qstring to avoid conflicting with Rusts standard methods.
Regarding: #[doc(hidden)] vs. free functions.
By convention, marking an item as #[doc(hidden)] removes it from the public API (see: https://predr.ag/blog/checking-semver-for-doc-hidden-items/ ), so we shouldn't make our lives more difficult by wrapping it in a free function to achieve that goal.
We should only use free functions when CXX doesn't support direct wrapping.
Whether we need to mark any to_qstring() functions as hidden I think isn't that important for this PR.
For the time being, you can just add them all as public API, that's fine by me.
If you want to hide them, that's fine by me too.
|
Hi @jnbooth sorry for the late reply. I'm still in favor of accessing the toString function directly. |
This PR adds
Debugimplementations to all types that a QDebug data stream accepts.Note: Some types incorrectly used QDebug output to implement
Displayinstead ofDebug. This PR does not remove those implementations because that would be a breaking change, but they probably should be removed for the next major release.