Differently from the previous patches, we don't parse nor export
comments associated with variables. This isn't a big deal because we
only export a single variable: virConnectAuthPtrDefault

Nonetheless, add version field to the exported XML by checking the
allowlist file. This way, if we add another variable in the future, we
can simply add it to that file. Calling the function should also warn
in case we are exporting a new Variable without adding to the file,
e.g:
    Missing 'Since' tag for: virConnectAuthPtrDefault

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 scripts/apibuild.py | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/scripts/apibuild.py b/scripts/apibuild.py
index 1235e75999..5493b3065e 100755
--- a/scripts/apibuild.py
+++ b/scripts/apibuild.py
@@ -2341,12 +2341,14 @@ class docBuilder:
 
     def serialize_variable(self, output, name):
         id = self.idx.variables[name]
+        # Only a single variable exported at the moment. Comments are not parser nor exported.
+        (_, since) = self.retrieve_comment_tags(name, "")
         if id.info is not None:
-            output.write("    <variable name='%s' file='%s' type='%s'/>\n" % (
-                name, self.modulename_file(id.header), id.info))
+            output.write("    <variable name='%s' file='%s' type='%s' version='%s'/>\n" % (
+                name, self.modulename_file(id.header), id.info, since))
         else:
-            output.write("    <variable name='%s' file='%s'/>\n" % (
-                name, self.modulename_file(id.header)))
+            output.write("    <variable name='%s' file='%s' version='%s'/>\n" % (
+                name, self.modulename_file(id.header), since))
 
     def serialize_function(self, output, name):
         id = self.idx.functions[name]
-- 
2.35.1

On Tue, Apr 05, 2022 at 01:43:23PM +0200, Victor Toso wrote:
> Differently from the previous patches, we don't parse nor export
> comments associated with variables. This isn't a big deal because we
> only export a single variable: virConnectAuthPtrDefault
>
> Nonetheless, add version field to the exported XML by checking the
> allowlist file. This way, if we add another variable in the future, we
> can simply add it to that file. Calling the function should also warn
> in case we are exporting a new Variable without adding to the file,
> e.g:
>     Missing 'Since' tag for: virConnectAuthPtrDefault

I think the fact that we're currently not parsing comments for
variables is just a mistake.

virConnectAuthPtrDefault seems to be documented in a way that would
be appropriate for the API docs, and the fact that it doesn't show up
there means that users of the library have no way to figure out what
it's there for without digging into the source code, or even that it
exists at all.

I'd say just start treating comments for variables the same as those
for all other symbols.

-- 
Andrea Bolognani / Red Hat / Virtualization

Hi,

On Tue, Apr 05, 2022 at 04:33:27PM +0000, Andrea Bolognani wrote:
> On Tue, Apr 05, 2022 at 01:43:23PM +0200, Victor Toso wrote:
> > Differently from the previous patches, we don't parse nor export
> > comments associated with variables. This isn't a big deal because we
> > only export a single variable: virConnectAuthPtrDefault
> >
> > Nonetheless, add version field to the exported XML by checking the
> > allowlist file. This way, if we add another variable in the future, we
> > can simply add it to that file. Calling the function should also warn
> > in case we are exporting a new Variable without adding to the file,
> > e.g:
> >     Missing 'Since' tag for: virConnectAuthPtrDefault
> 
> I think the fact that we're currently not parsing comments for
> variables is just a mistake.
> 
> virConnectAuthPtrDefault seems to be documented in a way that would
> be appropriate for the API docs

The documentation is not in the headers:

    https://gitlab.com/libvirt/libvirt/-/blob/master/include/libvirt/libvirt-host.h#L565

Only in the source:
    
    https://gitlab.com/libvirt/libvirt/-/blob/master/src/libvirt.c#L200

So, moving the documentation around could be an extra patch,
indeed.

> and the fact that it doesn't show up there means that users of
> the library have no way to figure out what it's there for
> without digging into the source code, or even that it exists at
> all.

Yes

> I'd say just start treating comments for variables the same as
> those for all other symbols.

TBH, because it was only a single exported variable, I didn't put
too much effort in parsing the docs, but you made a good point.
I'll try again, to parse comments from exported variables and
included it all in the XML API.

Cheers,
Victor

On Tue, Apr 05, 2022 at 08:31:40PM +0200, Victor Toso wrote:
> On Tue, Apr 05, 2022 at 04:33:27PM +0000, Andrea Bolognani wrote:
> > I think the fact that we're currently not parsing comments for
> > variables is just a mistake.
> >
> > virConnectAuthPtrDefault seems to be documented in a way that would
> > be appropriate for the API docs
>
> The documentation is not in the headers:
>
>     https://gitlab.com/libvirt/libvirt/-/blob/master/include/libvirt/libvirt-host.h#L565
>
> Only in the source:
>
>     https://gitlab.com/libvirt/libvirt/-/blob/master/src/libvirt.c#L200
>
> So, moving the documentation around could be an extra patch,
> indeed.

A lot of documentation lives in the .c file, notably that for
functions. It's perfectly fine for it to be there and it doesn't need
to be moved.

> > I'd say just start treating comments for variables the same as
> > those for all other symbols.
>
> TBH, because it was only a single exported variable, I didn't put
> too much effort in parsing the docs, but you made a good point.
> I'll try again, to parse comments from exported variables and
> included it all in the XML API.

Thanks!

-- 
Andrea Bolognani / Red Hat / Virtualization

Hi,

On Wed, Apr 06, 2022 at 01:10:40AM -0700, Andrea Bolognani wrote:
> On Tue, Apr 05, 2022 at 08:31:40PM +0200, Victor Toso wrote:
> > On Tue, Apr 05, 2022 at 04:33:27PM +0000, Andrea Bolognani wrote:
> > > I think the fact that we're currently not parsing comments for
> > > variables is just a mistake.
> > >
> > > virConnectAuthPtrDefault seems to be documented in a way that would
> > > be appropriate for the API docs
> >
> > The documentation is not in the headers:
> >
> >     https://gitlab.com/libvirt/libvirt/-/blob/master/include/libvirt/libvirt-host.h#L565
> >
> > Only in the source:
> >
> >     https://gitlab.com/libvirt/libvirt/-/blob/master/src/libvirt.c#L200
> >
> > So, moving the documentation around could be an extra patch,
> > indeed.
> 
> A lot of documentation lives in the .c file, notably that for
> functions. It's perfectly fine for it to be there and it doesn't need
> to be moved.

The suggestion of moving was to address what you pointed out
previously, that users of libvirt can't figure out what that
variable does without digging into the source code.

> > > (...) and the fact that it doesn't show up there means
> > > that users of the library have no way to figure out what
> > > it's there for without digging into the source code, or
> > > even that it exists at all.

So, If it is fine to leave documentation in non
exported/installed source/header files, I'll not be moving them
around.

Next version is just adding Since <version> where the
documentation is, let's see how it goes.

> > > I'd say just start treating comments for variables the same
> > > as those for all other symbols.
> >
> > TBH, because it was only a single exported variable, I didn't
> > put too much effort in parsing the docs, but you made a good
> > point.  I'll try again, to parse comments from exported
> > variables and included it all in the XML API.
> 
> Thanks!

Cheers,
Victor

On Wed, Apr 06, 2022 at 10:49:47AM +0200, Victor Toso wrote:
> On Wed, Apr 06, 2022 at 01:10:40AM -0700, Andrea Bolognani wrote:
> > On Tue, Apr 05, 2022 at 08:31:40PM +0200, Victor Toso wrote:
> > > The documentation is not in the headers:
> > >
> > >     https://gitlab.com/libvirt/libvirt/-/blob/master/include/libvirt/libvirt-host.h#L565
> > >
> > > Only in the source:
> > >
> > >     https://gitlab.com/libvirt/libvirt/-/blob/master/src/libvirt.c#L200
> > >
> > > So, moving the documentation around could be an extra patch,
> > > indeed.
> >
> > A lot of documentation lives in the .c file, notably that for
> > functions. It's perfectly fine for it to be there and it doesn't need
> > to be moved.
>
> The suggestion of moving was to address what you pointed out
> previously, that users of libvirt can't figure out what that
> variable does without digging into the source code.

Once the docstring for the variable is parsed and it shows up in the
HTML documentation, it becomes just as discoverable as any of the
functions, which is perfectly fine :)

-- 
Andrea Bolognani / Red Hat / Virtualization