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

Doxygen correction #9326

Merged
merged 1 commit into from Feb 13, 2019

Conversation

Projects
None yet
7 participants
@hasnainvirk
Copy link
Contributor

hasnainvirk commented Jan 10, 2019

Description

Adding to proper group so that the API doxygen appears into the class
hierarchy group rather than data strutures.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[X] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@melwee01 @AnotherButler

@ciarmcom ciarmcom requested review from AnotherButler , melwee01 and ARMmbed/mbed-os-maintainers Jan 10, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Jan 10, 2019

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-pan Jan 10, 2019

@melwee01

This comment has been minimized.

Copy link
Contributor

melwee01 commented Jan 10, 2019

Afraid I don't really know this structural stuff in doxygen well enough to comment on it.

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

hasnainvirk commented Jan 10, 2019

@melwee01 There are no structural changes here. Adding to a group merely forces the doxygen for these files to appear under Class Hierarchy rather than data structures.

@pan-
Copy link
Member

pan- left a comment

Few questions:

  • Could you explain the reason of the change ? Entities we wanted in the NFC group where enclosed into an addtogroup command; now it includes everything that is declared in the file (including namespaces) which is not always what we want.
  • What description comment doxygen pickup when it encounters these addtogroup statement ?
/** @addtogroup NFC
  * NFC Controller
  * @{
  */

// then 

/** @addtogroup NFC
  * NFC EEPROM
  * @{
  */

There is two very different comment for the same group: NFC Controller and NFC EEPROM. What is rendered ?

features/nfc/nfc/ndef/common/SimpleMessageParser.h Outdated
@@ -14,6 +14,10 @@
* limitations under the License.
*/

/** @addtogroup NFC

This comment has been minimized.

@pan-

pan- Jan 10, 2019

Member

The previous addtogroup command hasn't been removed.

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

hasnainvirk commented Jan 10, 2019

@pan- It says that the following file is under a group NFC and the module description is given in the next line. I am not sure why the previous format did not get parsed correctly by doxygen and that's why the documentation of NFC content went to the data structure section. Using the new format places it correctly under class hierarchy section. You can test it locally

doxygen doxyfile_options 2>&1 | grep features/nfc*

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:nfc_doxy branch Jan 10, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 16, 2019

@pan-
Copy link
Member

pan- left a comment

I tried it locally and the results are not good:

  • There is now two NFC modules: Nfc and NFC.
  • Data structures impacted by this PR does not appear in their module.
  • Additional text added in each addtogroup directive yield nonsense.

screenshot 2019-01-23 at 14 15 44

To document files, use the @file directive must be used.

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 12, 2019

@hasnainvirk Thoughts?

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:nfc_doxy branch Feb 12, 2019

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

hasnainvirk commented Feb 12, 2019

@pan- @cmonr Sorry for late response. @pan- Added corrections as you suggested. Please let me know if the changes are fine with you. Please feel free to commit directly to the PR fork if you think there is something missing, I am not familiar with these APIs.

features/nfc/nfc/NFCController.h Outdated
@@ -1,6 +1,8 @@
/* mbed Microcontroller Library
* Copyright (c) 2018 ARM Limited
*
* @file NFCController.h

This comment has been minimized.

@pan-

pan- Feb 12, 2019

Member

That is not going to do anything as it is not part of a Doxygen comment. The same is true for other headers modified.

This comment has been minimized.

@hasnainvirk

hasnainvirk Feb 12, 2019

Author Contributor

@pan- \file or @file [] should be standard doxygen specifier. Do you mean that there is some option that we need to turn on while generating doxygen in the doxy options file ?

This comment has been minimized.

@hasnainvirk

hasnainvirk Feb 12, 2019

Author Contributor

Interesting, I have seen many examples where file description is added alongwith license text. I can try to add a separate doxy section for @file.

This comment has been minimized.

@pan-

pan- Feb 12, 2019

Member

@hasnainvirk I'm not saying @file isn't a valid directive. I'm just pointing out it should be part of a Doxygen comment. A doxygen comment starts with /**, /*!, /// or //!. In this case the licence documentation block starts with /*

This comment has been minimized.

@hasnainvirk

hasnainvirk Feb 12, 2019

Author Contributor

Adjusted the license block instead.

This comment has been minimized.

@hasnainvirk

hasnainvirk Feb 12, 2019

Author Contributor

@pan- Yeah, figured that out while you were writing your message :)

Doxygen correction
Adding to proper group so that the API doxygen appears into the class
hierarchy group rather than data strutures.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:nfc_doxy branch to 2670eaf Feb 12, 2019

@cmonr cmonr added needs: CI and removed needs: review labels Feb 12, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 12, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 13, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 13, 2019

@ARMmbed/mbed-docs Heads up, doxygen change coming into 5.11.

@cmonr cmonr merged commit e84319f into ARMmbed:master Feb 13, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9925 cycles (+696 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 13, 2019

@pan-

This comment has been minimized.

Copy link
Member

pan- commented Feb 13, 2019

@cmonr Why did you merge it ? It hasn't been approved by the pan team.
The last fix is wrong as the documentation of the file will be the licence...

If you want to add a documentation block about the file the pattern must be:

/*
 * LICENCE .....
 */

/** 
 * @file <Filename> 
 * <Description>
 */

<File content>
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 13, 2019

@hasnainvirk can you please fix it?

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

hasnainvirk commented Feb 13, 2019

@pan- Does it matter ? I am not 100% but it shouldn't matter. Most of the public code would have \file \detailes \brief in the license text.

@pan-

This comment has been minimized.

Copy link
Member

pan- commented Feb 13, 2019

I'm sure it does matters if you look at the rendered documentation:

screenshot 2019-02-13 at 13 40 17

screenshot 2019-02-13 at 13 42 36

Does that looks good ?

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

hasnainvirk commented Feb 13, 2019

@pan- #9709 Please have a look at the fix.

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 13, 2019

My mistake @pan-

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.