-
Notifications
You must be signed in to change notification settings - Fork 559
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
Add doxygen marked-down source #586
Comments
I'm happy to do the documentation for |
That would be awesome @ukmo-ccbunney . I'll put your username next to those two so folks know it's claimed. done! |
I have started to add some doxygen markup to one of the F90 files and tried running doxygen, but I am having a few problems. Is there a minimum version of doxygen that is required? On my Linux box I have version 1.8.5 but it segfaults when I run it. The last few lines of output are here:
On my HPC, I have version 1.5.6 which seems to run, but the documentation for my module does not get generated correctly. It picks up the markup for the file ( Perhaps I am invoking it wrong? I am simply running Ta! |
Hi @ukmo-ccbunney , hmm, let me have a look. It normally goes off without a hitch, so I'll see if anything needs to be changed in the Doxygen.in. I'll post back momentarily.. |
@ukmo-ccbunney , try moving the Doxygen.in file up to the top level, then call Please let me know if that works or not. |
Ps, @ukmo-ccbunney I checked version on a linux vm I have and it's 1.8.17. I would think your version would be fine. If you want to push the branch your working on, I could try running it on my end to see if its due to the markup added, or something else like the environment |
Hmm...same issue unfortunately (exactly the same error message on version 1.8.5 on my Linux box). Like I say, it sort of works on the older version on my HPC, but my markup for the module is not appearing. Perhaps I am doing something wrong with the markup? I've pushed my changes up to a branch here: https://github.com/ukmo-waves/WW3/tree/doc/ounfmeta I've only added markup to the "file" (top level) and the "module" so far. Only the "file" markup is generating any documentation at the moment. If you can have a quick look then that would be great :) UPDATE: UPDATE 2:
|
@ukmo-ccbunney I'm having a look at the source right now, I'll give some feedback shortly |
@MatthewMasarik-NOAA Quick update - I have installed a new version of doxygen via Conda and it solves the issue I was having. I still get errors from some files, but it no longer segfaults. |
Interesting.. that's good to note the version may be important as others start documenting. I'm working with w3ounfmetamd.F90 and getting some similar behavior -- it's not producing output for file & module. Strange. I'll have it sorted out shortly |
I have it working for me now - have you tried my most recent commits? |
That's great! Oops, didn't have the most recent.. I'm glad to hear it's working. Also in your markup you mentioned how to document the module variables. I need to see if I had it right to use the |
Great - thanks @MatthewMasarik-NOAA . On a related note - do we have a policy on whether we are ditching the "WAVEWATCH III" header boxes, i.e. these:
I am hoping we can get rid of them, as they take a lot of space and are repeated for every module/subroutine/function. Also, are we keeping the change log in the comments, or are we going to rely on Git's commit log for this? @aliabdolali and @JessicaMeixner-NOAA - do you have any opinions on this? |
For now the doxygen is just an addition and we're keeping the normal WW3 header and logs. I'd be open to changes though. I'm not a huge fan of the log personally but there are a few comments here/there that are valuable. I'd hope we could keep the important parts of the descriptions. |
OK - I have deleted the sections that relate to the new Doxygen tags though (i.e. "1. Purpose", "2. Method", "3. Parameters", etc. I have left the header and the change log though. Should have I left the others untouched too? |
I'd agree with @JessicaMeixner-NOAA , we had some discussions on this. I believe leaving the original headers intact (no markup) and adding some (relatively brief) doxygen markup above them is best. As you noted, the source below the tags is all displayed right below the marked up information. |
The approach I have been using is to leave the header completely untouched, then copy (or summarize in 1 sentence) the 1. Purpose content into the @brief tag. For the @details, I was putting a couple summarizing sentences from 2. Method, or elsewhere in the header that seemed helpful. I shoot for ~1 paragraph of info. I think there's roof for individual taste here though. Does that get at what you're asking? |
*roof = room |
Yeah - that's cool and pretty much what I have done. I will reinstate the original comments. However, if you agree, I will not keep the original comment for the module itself in w3ounfmetamd. It is a very long description of the module and I have moved it to the doxygen |
That seems just fine to me. For people documenting their own code you should have some license to how you want it. If you're documenting a someone else's code, we may want to be judicious with what is removed. I think what you mentioned sounds good |
To follow up regarding the module variable documentation, do use inline comments where the variables are declared, and not with a @param tag in the doxygen header. That's my error in the Reference. I'll make a PR to correct that. Below I copy/pasted a comment from a closed PR in which @edwardhartnett shows this: The !< marker can be used when var documentation is short enough to fit on one line, but is not mandatory. Just as with other doxygen documentation blocks, the !> marker can be used, and the documentation placed before the variable. It should be noted that all module and type variables need documentation. That is, each variable must be documented, none skipped (or you will get warnings). Here's an example using the !< marker, where each element of documentation fits on one line:
Here's an example where more detail is required, and the documentation elements usually come before the variables:
|
@MatthewMasarik-NOAA - Okey-dokey. |
Yes, exactly correct. |
In general the doxygen version is significant. 1.8.5 is very old and should be upgraded. The reason includes the fact that the Doxyfile changes from version to version - new items are added, and some are taken away. We rely on several settings that are in more recent doxygen versions, including WARN_AS_ERROR. When we set up the doxygen build in the CI, it will be using the doxygen release associated with "ubuntu-latest" which, currently, is 1.8.17. Everyone using doxygen on their own machine should upgrade to 1.8.17. Do not upgrade to 1.9.x, because that is not what will be used in the CI. So 1.8.17 is the canonical version of doxygen at the moment. In general, close versions of doxygen usually work, though sometimes with extra warnings. But the doxygen build must target a specific version, which is currently 1.8.17. On NCEPLIBS we converted all old logs to tables in the doxygen, for example:
This yields an attractive output, and is compact in the code. In NCEPLIBS we took out all other headers and documentation blocks, and this is the common practice with doxygen projects. The goal is to have only what is needed, and, as was mentioned above, having some boilerplate headers at the top of every file is not useful. (We also have our legal disclaimer at the bottom of every README.md file.) If there is a desire to have some repeated text appear everywhere, that can be done in doxygen, as a page header or footer, or in some other ways. Doxygen also has special handling for copyright information, should you wish to use it. (That is, you can define a copyright block and include it wherever you like). In general, the text you want to repeat in the output documentation does not have to be repeated in the code everywhere. |
Hey all, if you have completed files to add, you can commit them to the draft PR I just opened: #600 . |
@edwardhartnett I like the change history table idea. I will use that for the top level change log in the module. However, we have change logs on each individual function/subroutine too. I think this is a bit over the top in the documentation, so I won't elevate those to doxygen comments. |
@MatthewMasarik-NOAA I don't have write access to your branch, so can't add my changes. I've got some files ready to go in my https://github.com/ukmo-waves/WW3/tree/doc/ounfmeta branch; shall I raise my own PR for them? I would like your (or @edwardhartnett 's) input on one issue I have had though (for which I have a workaround that doesn't make sense to me!) See the comment here: |
Since this branch does not seem to have the CMake build, how do you build the docs? |
I run Edit: This is branched from develop - not the cmake branch. |
Hi @ukmo-ccbunney , I'm back from being away last week. I'm going to take quick look at your branch to see if something sticks out.. Do you have files to add to the PR I made? Update: I'll pull in your doc/ounfmeta branch. |
@ukmo-ccbunney I found how to fix the
I'll PR this change. |
@MatthewMasarik-NOAA I am so glad you found that, I was totally on the wrong track with this one! ;-) And I should have remembered this because it came up in another project. by default *.f90 is accepted as free-format Fortran, but not *.F90! |
Thanks @MatthewMasarik-NOAA I've just pushed one small change to remove the comment line from w3ounfmetamd.F90. BTW - I tried to raise a PR to target your branch, but your WW3 repo doesn't appear in the list of available target forks...which is odd. If you're happy to pull in my changes, then that's great. |
@ukmo-ccbunney It is odd, I'm working to get that rectified. I'll let you know it's fixed, in the time being I'm happy to pull in your changes. |
@MatthewMasarik-NOAA |
@ukmo-ccbunney that would be great. I just added you as a collaborator to the repo. See if that solved the issue... if not, I'll put you down for those files. |
I can't edit your PR description, but I can now raise a PR against your repo (although I have not done so yet), so it does fix that issue. If you can update the description with my name against those files I will raise a PR when I am done. Thanks! |
Okay, that is good news you can raise a PR now.
sure thing! I've got those assigned to you. |
Hi @aronroland @MathieuDutSik, do either of you have interest in adding doxygen documentation to the .F90 files in |
Hi Mathew, yes definitely this must be done, we started it long time ago but it was laid down. I will do that asap. Thanks for looking at this! |
Hi Aron, that's great news! Thank you. |
💯 |
@MatthewMasarik-NOAA and I discussed this doxygenation effort as part of the larger EMC documentation effort. Excellent documentation is a requirement for all EMC science codes. Good documentation speeds development, prevents bugs, and greatly assists testing efforts, especially when tests are being written by those who did not originate the code. Good documentation is all that stands between a programmer and the misuse of his or her code, causing an expensive or embarrassing problem which could easily have been avoided. We have upgraded the documentation to doxygen, with high standards, on all the NCEPLIBS, UFS_UTILS, UPP, and soon fv3atm. It's great to see it happening on WW3 and we are ready to help out, as we did with the other projects. The ultimate goal, which takes effort, but is well worth it, is to turn on doxygen checking in the CI system. Currently, for NCEPLIBS or UFS_UTILS, if a contributor submits a PR with undocumented code, it will fail CI. This provides immediate feedback to the programmer and allows them to resolve the problem cheaply and immediately. They can submit a PR at 3 am, notice it failed, and have the documentation updated by 3:15 am. All without involving Matt or any other team member. This is how other science software projects maintain excellent documentation with very few programmers. We automate as much as we can, to save team time to work on important programming problems. |
Add Doxygen Documentation
For each file, document each contained Fortran code units (file, module, subroutine, etc.) using the agreed upon doxygen headers:
The text was updated successfully, but these errors were encountered: