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.

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

[Doc] Documentation for FDB.h #49

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tbkr
Copy link

@tbkr tbkr commented Oct 29, 2024

Added some documentation for the C++-API of the FDB class. There are still some open questions linked to some of the functions.

This is a first draft.

@FussyDuck
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.10%. Comparing base (72fd6af) to head (3e69ae6).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop      #49   +/-   ##
========================================
  Coverage    64.10%   64.10%           
========================================
  Files          238      238           
  Lines        13656    13656           
  Branches      1313     1313           
========================================
  Hits          8754     8754           
  Misses        4902     4902           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tbkr tbkr force-pushed the feature/c++-api-documentation branch from 44a3f30 to 497a12e Compare October 29, 2024 09:17
Copy link
Contributor

@mcakircali mcakircali left a comment

Choose a reason for hiding this comment

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

overall looks good to me
throws could be added if any
minor thing: @TODO -> @todo

@tbkr tbkr force-pushed the feature/c++-api-documentation branch 2 times, most recently from f2aea4b to 5c31fff Compare October 29, 2024 10:41
Added some documentation for the C++-API of the FDB class.
There are still some open questions linked to some of the functions.

This is a first draft.
@tbkr tbkr force-pushed the feature/c++-api-documentation branch from 5c31fff to 3e69ae6 Compare October 29, 2024 10:49
@ChrisspyB
Copy link
Member

ChrisspyB commented Oct 29, 2024

For readability, I'd definitely like a dash (or colon, or some kind of separator) after a \param's keyword. e.g.:

Before: \param doit flag for committing to the wipe (default is dry-run)
After: \param doit - flag for committing to the wipe (default is dry-run)

Edit: I notice you did have that, and removed it. I personally strongly prefer the presence of a separator

@mcakircali
Copy link
Contributor

For readability, I'd definitely like a dash (or colon, or some kind of separator) after a \param's keyword. e.g.:

Before: \param doit flag for committing to the wipe (default is dry-run) After: \param doit - flag for committing to the wipe (default is dry-run)

Edit: I notice you did have that, and removed it. I personally strongly prefer the presence of a separator

Your suggestion means - should be part of the description, which becomes problematic.

e.g., using vscode, there is already - in the mouse-over previews.. and if a - is part of description, it looks like - - in previews.

I would strongly suggest against using - manually as part of a description.

@@ -71,72 +71,253 @@ class FDB {

// -------------- Primary API functions ----------------------------

/** Archive binary data to a FDB.
*
* \param handle eckit::message::Message to data to archive
Copy link

Choose a reason for hiding this comment

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

The types can be omitted from the description. IMO this is just hard to keep in sync with the actual implementation

eckit::DataHandle* read(const eckit::URI& uri);

/** Read binary data from an list of URI
*
* \param vector of uris eckit uris to the data source
Copy link

@Ozaq Ozaq Nov 1, 2024

Choose a reason for hiding this comment

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

this should be \params <name or parameter> i.e. \params uris

DumpIterator dump(const FDBToolRequest& request, bool simple=false);

/// TODO: Is this function superfluous given the control() function?
// \todo Is this function superfluous given the control() function?
Copy link

Choose a reason for hiding this comment

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

I think this was supposed to be a code comment TODO and not a doc comment TODO

Copy link

@Ozaq Ozaq left a comment

Choose a reason for hiding this comment

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

I don't know if anyone has a preference but this are the supported styles: https://www.doxygen.nl/manual/docblocks.html

  1. Java doc
/** Just a brief */

/**
 * A brief.
 * With a detail section
 */ 

--OR--

/**
   A brief.
   With a detail section
 */ 
  1. QT style doc
/*! Just a brief */

/*!
 * A brief.
 * With a detail section
 */ 

--OR--

/*!
   A brief.
   With a detail section
 */ 
  1. C#(ish) style
/// Just a brief

/// A brief.
/// With a detail section

--OR--

//! Brief

//! Brief
//! With a detail section 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants