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

RFC: Use doxygen for generating API docs #29

Merged
merged 1 commit into from
Mar 23, 2015

Conversation

jonas
Copy link
Contributor

@jonas jonas commented Mar 18, 2015

Hi,

To address issue #26, I've started working on converting the header file so doxygen can be used to generate API docs. It's not complete yet, hence the "RFC" prefix. I would however like to know if the general direction is good. A couple of things to consider:

  • The comments are quite invasive, especially for the #define statements. If it is preferable, the comments could go before and simply reference the symbols they document.
  • I'm using markdown as much as possible to keep the comments readable.
  • Comments follow the initial formatting of utf8proc (recent comments didn't follow this):
/** Some title.
 *  Note the two spaces before the main text in the comment.
 */
  • @return and @param are used, although it requires that the comments are restructured.

You can see the docs here: http://jonas.nitro.dk/utf8proc/html/index.html
With the important part being: http://jonas.nitro.dk/utf8proc/html/utf8proc_8h.html

I've also briefly looked at whether to use Sphinx for generating docs through the breathe plugin. This might allow us to create better looking docs. I don't know if that would be of interest, else I also found a way to use bootstrap with doxygen to make it look more modern.

[ I'm considering using utf8proc in tig so I am very excited that you have taken over maintaining this great library. ]

@jonas
Copy link
Contributor Author

jonas commented Mar 18, 2015

Forgot to mention that I didn't integrate anything into the build system for now.

@tkelman
Copy link
Contributor

tkelman commented Mar 18, 2015

Cool! This is probably a case of "he who implements it gets to decide what tool to use," but this looks not bad at all. Would breathe allow us to host everything on readthedocs? Judging by http://cppformat.readthedocs.org/en/stable/ as an example, that does look a lot nicer than the doxygen default.

@jiahao
Copy link
Collaborator

jiahao commented Mar 18, 2015

I'm quite used to doxygen, so I wouldn't mind.

I would make a few small changes:

@@ -242,7 +242,7 @@ TCL_SUBST              =
 # members will be omitted, etc.
 # The default value is: NO.

-OPTIMIZE_OUTPUT_FOR_C  = NO
+OPTIMIZE_OUTPUT_FOR_C  = YES

 # Set the OPTIMIZE_OUTPUT_JAVA tag to YES if your project consists of Java or
 # Python sources only. Doxygen will then generate output that is more tailored
@@ -793,7 +793,7 @@ RECURSIVE              = NO
 # Note that relative paths are relative to the directory from which doxygen is
 # run.

-EXCLUDE                =
+EXCLUDE                = NEWS.md lump.md

 # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or
 # directories that are symbolic links (a Unix file system feature) are excluded

@jonas
Copy link
Contributor Author

jonas commented Mar 19, 2015

@tkelman Yes, as I understand it, breathe allows doc hosting on readthedocs. I didn't actually try to set it up yet. In any case, Doxygen is required to "lift" the docs from source.

@jiahao Thanks. Was not sure whether the external files made sense.

@stevengj
Copy link
Member

Maybe the #define statements should be replaced by enums?

Thanks for doing this; improving the documentation was the last thing I wanted to do before releasing utf8proc-1.2, and I agree that Doxygen is the natural choice here.

There is no particular reason to stick with the original commenting style of utf8proc, since we are now the upstream — it would be better to use whatever is conventional for Doxygen.

* STABLE: prohibits combining characters which would violate
* the unicode versioning stability
*
* @param buffer the sequence of unicode characters to reencode.
Copy link
Member

Choose a reason for hiding this comment

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

the (native-endian UTF-32) unicode codepoints to re-encode

@jonas
Copy link
Contributor Author

jonas commented Mar 21, 2015

Thanks for the comments. I've tried to address the suggestions in the additional commits. I kept the changes separate so that they can easily be included or dropped in a final squashed commit.

Regarding the enum's it really cleans up the docs a lot. I had to use a typedef for the categories to not conflict with the utf8proc_category method and decided to also use a typedef for the options to keep things consistent.

} utf8proc_category_t;

/** Bidirectional character classes. */
enum utf8proc_bidi_class {
Copy link
Member

Choose a reason for hiding this comment

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

typedef enum { .... } utf8proc_bidi_class_t for consistency?

@stevengj
Copy link
Member

Great, can you update your doc page so that we can see what it looks like?

@jonas
Copy link
Contributor Author

jonas commented Mar 21, 2015

@stevengj It's updated.

@@ -12,6 +12,7 @@
data/*.txt
data/*.ttf
data/*.sfd
/docs/
Copy link
Member

Choose a reason for hiding this comment

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

What is the docs directory for? The generated docs? I'm not sure we should distribute those in the tarball as opposed to just posting them.

Copy link
Member

Choose a reason for hiding this comment

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

Distributions prefer shipping docs with packages rather than only online. That's required some work in Julia, so if it works here please keep them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It goes with this line in Doxyfile:

+OUTPUT_DIRECTORY       = docs

It contains docs/html, docs/man etc.

@stevengj
Copy link
Member

Looks good, can you squash the commits for merging?

@jonas
Copy link
Contributor Author

jonas commented Mar 22, 2015

@stevengj Squashed. I also added 'Fix #26' to the commit title.

stevengj added a commit that referenced this pull request Mar 23, 2015
RFC: Use doxygen for generating API docs
@stevengj stevengj merged commit 3a9eac4 into JuliaStrings:master Mar 23, 2015
@stevengj
Copy link
Member

Now, the next steps:

  • How do we autogenerate the docs for the github-created tarballs?
  • How do we autogenerate the docs for readthedocs or similar, so that the online docs are auto-updated in sync with github?

@nalimilan
Copy link
Member

No idea about the second point, but as regards GitHub-generated tarballs, I think they're best avoided. Usually, projects have a make dist or make distcheck target which builds everything that is needed an creates the tarball. A good make distcheck tries to build the resulting tarball and run the tests from it, so that you can be sure you didn't forget a file in the release, or some silly thing like this.

I'm not saying this is very high priority, but that's what I see as a good practice.

@jonas
Copy link
Contributor Author

jonas commented Mar 24, 2015 via email

@stevengj stevengj mentioned this pull request Mar 26, 2015
stevengj added a commit that referenced this pull request Mar 26, 2015
@PallHaraldsson PallHaraldsson mentioned this pull request Oct 24, 2023
1 task
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.

5 participants