Skip to content

Doxygen for source #5213

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Doxygen for source #5213

wants to merge 3 commits into from

Conversation

c1728p9
Copy link
Contributor

@c1728p9 c1728p9 commented Sep 27, 2017

Generate doxygen for source file in addition to header files. Also generate doxygen for tests.

@c1728p9
Fix doxygen error in mbed_retarget.cpp
4556a3e 
Add add documentation to the second parameter of handle_open_errors.
This is in preparation for turning on doxygen for source files.
This was referenced Sep 27, 2017
Closed
Closed
@c1728p9
Enable doxygen for source files
df84eec 
Turn on doxygen for .c and .cpp files. Ignore the source directories
in frameworks, as several of these files contain doxygen errors.
@c1728p9
Include doxygen from test directories
10985ee 
Turn on doxygen for tests. This allows testing functions to be
referenced, which will be used for various parts of the HAL
specification.
@c1728p9
Copy link
Contributor Author

c1728p9 commented Sep 27, 2017

/morph test

1 similar comment
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 28, 2017

/morph test

0xc0170
0xc0170 approved these changes Sep 28, 2017
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Not certain about source files doxygen, it is in most cases they contain internal detail - implementation specific - should this be generated?

@pan-
Copy link
Member

pan- commented Sep 28, 2017

Could you explain what are the benefits of these changes ?
If I understand correctly, API from an header in the TEST folder and therefore available only in this test context will be included in the OS documentation. Should we have rules about documentation in the Test folder ?

@bulislaw
Copy link
Member

Why do we want to do that? Can we put all porting/implementation relevant documentation only in API headers and the porting guide?

@c1728p9
Copy link
Contributor Author

c1728p9 commented Sep 28, 2017

If the HAL specification is going to reference the test functions which validate defined behavior then these functions need to be in doxygen somewhere. This could be:

  1. Dedicated test header files
  2. The API headers
  3. The test source files

Several people seemed to be against option 1 of having dedicated test header files and I don't think that filling the API header with a lot of test function prototypes (option 2) would be any better. This leaves option 3, which is why I created this PR to turn on doxygen in the test directory. If you know of any other option, or think it is unreasonable to reference the test functions from doxygen, please let me know.

There isn't must of a downside to building doxygen for source files. If you don't explicitly add it to a group it won't effect mbed-os's doxygen. Additionally, CI will be able to catch incorrectly formatted doxygen in source files.

If you have concerns with how doxygen will look because of this change, I highly recommend that you build doxygen for yourself and take a look. To do this run doxygen (as shown below) from the mbed-os directory and open BUILD\html\index.html.

> mkdir BUILD
> doxygen doxyfile_options

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1419

All builds and test passed!

@theotherjimmy
Copy link
Contributor

@pan- @bulislaw Are you happy with the answer to your questions?

@bulislaw
Copy link
Member

bulislaw commented Sep 29, 2017 •
edited
Loading

Ok, I think I misunderstood your goal before, sorry for that! I'm not against having test headers, actually they makes it easier to see what we actually test, when the test cases are not mixed with the test code. I'm ok with referencing test cases in HAL specification, as long as they don't provide more information required to complete the port.

What I don't want to have is yet another source of porting information, that can go out of sync, PG and the HAL headers are enough. They should contain all the require informations and details necessary to successfully port mbed OS. We can quote tests that validate each of the assertions I'm fine with that.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 29, 2017

Several people seemed to be against option 1 of having dedicated test header files and I don't think that filling the API header with a lot of test function prototypes (option 2) would be any better. This leaves option 3, which is why I created this PR to turn on doxygen in the test directory. If you know of any other option, or think it is unreasonable to reference the test functions from doxygen, please let me know.

I would also consider those 2 (1st or 3rd).

What others think, green or red for this? @bulislaw @pan- @deepikabhavnani @maciejbocianski @fkjagodzinski

@deepikabhavnani
Copy link

Header file approach is used instead of source in mbed-os, so I would say let us stick to that and add test headers separately. But API headers and test header should not be mixed.
I am fine with approach 3 as well. But not with 2.

@c1728p9
Copy link
Contributor Author

c1728p9 commented Sep 29, 2017

Approach 3, test function doxygen in source in #5008
Approach 1, test function doxygen in headers in #5226

@bulislaw, @0xc0170, @deepikabhavnani please let me know what you think of the two alternative implementations (or if you have other ideas).

Some pros of having doxygen run on source files:

  • CI will check to make sure doxygen is valid in source files (in addition to headers) if present
  • Extra header files to reference tests from doxygen are not required
  • Doxygen is aware of and can link to functions in source

Some cons of having doxygen run on source files:

  • Adds ambiguity as to whether doxygen should live in headers or source files or both
  • Doxygen cluttered with functions from source Functions won't show up in modules unless explicitly added to a group

@fkjagodzinski
Copy link
Member

Personally I like the 3rd option. The simpler the better -- test headers look like an overhead for me.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 2, 2017

I would vote for 1st (consistency with mbed-os).

@bulislaw
Copy link
Member

bulislaw commented Oct 2, 2017

I don't have strong preferences, but I think having a test plan in headers make it easier to read as it adds separation from test code. So I would vote for the 1st.

@deepikabhavnani
Copy link

I will go with approach 1

@c1728p9
Copy link
Contributor Author

c1728p9 commented Oct 2, 2017

I created #5238 to include doxygen for test directories and enable approach 1.

Any preferences on if doxygen should be turned on for source? If this isn't a desirable feature then I'll close this PR.

@bulislaw
Copy link
Member

bulislaw commented Oct 3, 2017

I don't think we need this changes.

@c1728p9 c1728p9 closed this Oct 4, 2017
@sg- sg- removed the needs: review label Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@c1728p9 @0xc0170 @pan- @bulislaw @mbed-bot @theotherjimmy @deepikabhavnani @fkjagodzinski @sg-