Copy docs to getter in Properties macro

[carlosmn]

Spurred by #1236 though there's probably more we can do here.

The change here is that we take anything that's a doc before a #[property] and consider that to be the docs for the getter, as it seems like the getter and the property would be the most likely to share doc comments.

There is of course an open question of what one could do to get docs for the setter that aren't a pain to write, but at least for the setter we can make something that, I think, makes intuitive sense.

The big issue here is that you wouldn't get docs copied over if you add a doc to the field but after the #[property]. This is probably something we could allow, but I don't know how important it would be.

[bilelmoussaoui]

I am fine with the changes but we need to first come up with at least an approach on how to handle the setters/notify functions docs. Could we just auto-generate them based on Set $this_property or something like that?

[carlosmn]

By Set $property name do you mean if a line starts with that text, we'd consider the rest of the doc comments to be part of the setter? That is maybe a bit subtle but it does let you write a normal comment.

I wonder if a pseudo-attribute would work, something like

/// Here I am writing my comment for the getter
///
/// #[setter]
///
/// And now I'm writing docs for the setter.

Then again this works the other way around to how you write doc comments so maybe it's the other way around that matches how you would expect it to be in actual Rust. Or there's also adding more actual attributes for us to read, though I haven't checked how awkward it would be to parse this, something like

/// Comments for the getter
#[getter]
/// And for the setter
#[setter]
#[property(get, set)]

though it makes it more awkward to write.

I don't know how possible it is (and it would be really ugly, I think) but you could also put the comments where you define that you have a getter and setter

#[property(
/// Some comment for the getter
get,
/// And a comment for the setter
set]

but maybe that's just too ugly and awkward a syntax to ask people to write.

[bilelmoussaoui]

Tbh, I don't have any ideas here that is why I didn't reply earlier. I just don't want people to refactor a bunch of code in case the to be added setter support makes the getter docs behave differently.

[carlosmn]

Yeah, there's no obviously good answer unfortunately.

A variant I just though of is using /// # Setter as the context change (or allow /// # Getter too, whatever we end up with). This would a little bit like the /// # Safety block that clippy will warn you about for unsafe functions. There is #1490 (comment) and then also https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_doc for when it's unnecessary. That sets a bit of a precedent that the tooling can take markdown headers and treat the them as machine-readable.

Of course that's only a little bit like this as clippy isn't doing anything beyond seeing that the line exists, and it doesn't affect how the docs generate anything. So it's not the strongest precedent, but having

/// Comments for the getter
///
/// # Setter
///
/// And for the setter
#[property(get, set)]

seems like it flows when writing it and it's something you can parse and relate to other cases (though maybe not all that many people are actually writing unsafe functions).

[bilelmoussaoui]

I would say using # Getter and # Setter would be good. As we also have # Notify for the generted notify_$property. And leave any comment before that relevant to the field as it might be some in code explanation

[carlosmn]

Ok that seems reasonable, so we'd end up like

/// Some comments that remain internal
///
/// # Getter
///
/// Docs that land in the getter
///
/// # Setter
///
/// And docs for the setter
#[property(get, set)]

[carlosmn]

I'm now thinking that we have two kinds of docs here: in one situation you're fine with the same doc on both setter and getter, and then sometimes it's important to specify something about the getter or setter. So alongside the headings, I think it makes sense to allow for the simple case of just copying the doc to both setter and getter if you don't opt into defining them.

[carlosmn]

Ok well I finally found time/motivation to look back into this and just ended up taking a few hours.

This now implements what we discussed as a reasonable compromise which includes an example so you can check it while messing around with the code.

If this does seem like we want then these comment copy rules and example should end up in lib.rs too. I might tackle that that in the next few days.

[bilelmoussaoui]

Would add Notify support as well while at it

[carlosmn]

That makes sense yeah. Though I think it'll have to be in a different function because we call this one from within the loop inside expand_impl_getset_properties and the notify code is generated from a different one.

[carlosmn]

I added # Notify to copy the docs into notify_$property but now I'm not sure if that's what you mean or if it was supposed to be the connect_$property_notify as this one is maybe more likely to be something that a user of a class would need documentation about when that triggers or what the data would mean there.

[bilelmoussaoui]

I would auto-generate the connect_property_notify docs based on the notify one but not sure

[carlosmn]

How often does one want to document the notify_$property method itself? Wouldn't that always just say to notify the property? I suppose you could mention when to call it as a user of the class, but I don't know how common that is for public types.

But the connect_$property_notify method I think would benefit much more commonly from docs, no? You might want to specify non-obvious times it would get called, maybe values that depend on other values?

I don't have experience with maintaining a public package, but looking at the docs for gtk itself, e.g. https://docs.gtk.org/gdk4/class.Surface.html (not that we have to match C but consistency is nice) the docs for the signals seem like the connect_ side of the signals rather than the emitting side. Maybe it should be a # Signal heading instead? That would also make it match tighter if we end up making it so there's some sort of list of signals in the docs.

[carlosmn]

It looks like I was conflating explicitly-defined signals and property change notifications, as the latter is not spelled out in the docs, so now I'm not sure.

[bilelmoussaoui]

Yes, because C doesn't generate a "safe" wrapper for signals compared to Rust bindings that is why there is no documentation for anything. Even less for the notify signal.

I would say, let us just skip having a special header for both Notify and the connect_notify and instead manually amend something like "Notify changes for property X" and "Listen to changes of property X".

[carlosmn]

OK, I've removed the handling for notify and added generic comments that just mention the notification name. We can always add some more "instructions" to the comment if this is something people want to do.

[bilelmoussaoui]

Otherwise this looks good to me!