Review Request 119997: Add documentation to KConfig::sync()

Fri Sep 12 02:36:09 UTC 2014

> On Aug. 31, 2014, 12:57 a.m., Matthew Dawson wrote:
> > Thanks for taking a look at this.  It appears KConfigBase isn't available on api.kde.org as it isn't documented, as kapidox hides such classes by default.  As KConfigBase is used outside of KConfig, I'd prefer if KConfigBase gained a quick blurb to copying its documentation.  I think something describing it as a way for a function to accept either a KConfig or a KConfigGroup and be able load/persist its settings would be good.
> 
> Martin Klapetek wrote:
>     KConfigBase is very much documented, see [1]. I don't know why it's not showing up...nevertheless, if that method is reimplementation of the base class method, it should imho has its own documentation altogether (it's *reimplementation* after all). What I did here is just a quick fix to improve that...
>     
>     [1] - http://quickgit.kde.org/?p=kconfig.git&a=blob&h=3d00d98a1eb565df1a6eea2eba165bfeda17978b&hb=5941a608038d799244ba2dfdceb2da8bf1685fc1&f=src%2Fcore%2Fkconfigbase.h
> 
> Matthew Dawson wrote:
>     While class members of KConfigBase are documented, Doxygen doesn't consider it documented as the class itself does not have a description attached.  Attaching even a brief line convinces Doxygen otherwise.
>     
>     Regarding each method having its own documentation, I took a quick look through the other code I have checked out on my machine, and it appears most classes don't copy the documentation from their parent.  I couldn't find any policy on this either from KDE's website.  If we want each function in frameworks to be fully documented, without having to click a reference, then I'm fine with this patch.  This should then probably get documnted on the community wiki, so others know for the future to include it.
>     
>     However, if this patch came from the fact KConfigBase is not on api.kde.org, then I rather fix this by throwing a quick description on KConfigBase.  Then the sync method's documentation does show up, and a link is generated on KConfig's sync method making it easy to read.  Maybe something like:
>     
>     \brief Interface to interact with configuration.
>     
>     KConfigBase allow a component of an application to persist its configuration, without having to care if it persists its data into a top level KConfig, or a KConfigGroup inside a KConfig.
> 
> Martin Klapetek wrote:
>     > it appears most classes don't copy the documentation from their parent.
>     
>     Right; my point is that a reimplementation of an abstract class method/interface method is usually a specific implementation of the broad parent abstract class meaning.
>     
>     > If we want each function in frameworks to be fully documented... if this patch came from the fact KConfigBase is not on api.kde.org
>     
>     In the light of what I wrote above, my intention was a little bit of both.
> 
> Aleix Pol Gonzalez wrote:
>     Well, if @reimp is passed, it would mean that the parent documentation is good enough, no?
> 
> Martin Klapetek wrote:
>     I thought that "@reimp" in doxygen marks the method as reimplemented from the parent class, ie. it adds a link to it. No?
> 
> Matthew Dawson wrote:
>     I agree that if the specific implementation have further notes that the parent's documentation does not address, it would be good to document it.  However, the current patch only copies the documentation from KConfigBase.  KConfigBase isn't on api.kde.org it is not considered documented by Doxygen (regardless of what Doxygen tags are attached now).  Attaching a quick couple lines to KConfigBase would make it visible, avoiding the copied comment.  If my suggestion above is fine, then I'll commit it.  If there is further documentation that should be attached to KConfig::sync, this RR can be updated appropriately.
>     
>     I do think it would be good to attach some extra documentation to KConfigGroup::sync, mentioning it syncs the entire file the group is from, as that might surprise some people.  That can be included either in this RR, or I can add something with KConfigBase's documentation commit.
> 
> Martin Klapetek wrote:
>     > However, the current patch only copies the documentation from KConfigBase
>     
>     Yes, I wanted to do a "quick fix" and as I don't know much about the internals, I just copied the base class' docs, which seems fitting enough.
>     
>     Ultimately, you're the maintainer, so it's up to you if you add docs to KConfigBase or extend the sync() comment properly. I mostly wanted to point out the missing docs and have it fixed quickly. I do agree that it's not the bestest proper fix ever and I'm happy to discard it for more favorable solution :)

Alright, I've pushed a commit (http://commits.kde.org/kconfig/fe300454292c519ae5aa4edfcb1b94f3636e59bb) so KConfigBase shows up (and a link now exists between KConfig::sync and KConfigBase::sync).  KConfigGroup::sync already had a note, so I didn't change it for now.  If you feel it can be helped (the KConfig::sync link isn't obvious, unfortunately), please update this RR.  Otherwise please discard this one.

- Matthew

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119997/#review65567
-----------------------------------------------------------

On Aug. 29, 2014, 5:42 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119997/
> -----------------------------------------------------------
> 
> (Updated Aug. 29, 2014, 5:42 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> The docs at api.kde.org shows just "Reimplemented from superclass." for the sync() method, however the superclass, KConfigBase, is not available at the api.kde.org. So this copies the documentation from KConfigBase to KConfig so the text can actually be visible at api.kde.org
> 
> 
> Diffs
> -----
> 
>   src/core/kconfig.h d7d4b7d 
> 
> Diff: https://git.reviewboard.kde.org/r/119997/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140912/b712b191/attachment.html>