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

[docs] Properly document Python API removed functions in changelog #14584

Merged
merged 1 commit into from
Oct 16, 2018

Conversation

enen92
Copy link
Member

@enen92 enen92 commented Oct 11, 2018

Description

To have a complete changelog for the Python API we need to take into account method definitions that were removed from the API. Those methods should somehow also point to the method definition in older revisions of the documentation since they are no longer available.

Motivation and Context

While browsing our old python documentation for the RenderCapture module I noticed (apart from the broken stuff) that two methods were not available in our current docs: getCaptureState() and waitForCaptureStateChangeEvent(). By tracking what happened with them, I found the documentation was there but, as both C++ function definitions were removed, it was annotating an inline function that did not belong to the API (GetPixels()).
Furthermore, the removal comments were also pointing to this function in the changelog of Kodi v17 (see image below).

As in the future the python API is likely to change with more functions being removed I decided to create a new page to document such functions (which mimics the same layout of current API functions) allowing Python devs to have a complete changelog (and find out how to use the method in older Kodi versions). In this new page, when a method is completely removed (including the function definition) it can be documented like below

To avoid creating a new page or introducing boilerplate methods, I decided to create a new doxygen command to document removed functions. Those entries should be added to the specific page where the removed functions apply (e.g. @page python_v17 Python API v17). To add a new function just do something like exemplified below:

\python_removed_function{
      getCaptureState,
      http://mirrors.kodi.tv/docs/python-docs/16.x-jarvis/xbmc.html#RenderCapture-getCaptureState,
      <b>xbmc.RenderCapture().getCaptureState()</b> function was removed completely.
}

Where each field represents the following:

  • The name of the removed method
  • The link to the method documentation in prior revisions of our docs
  • The message to display in the changelog

This structure mimics all the other changes already available in the page resulting in the entries illustrated in the screenshots

How Has This Been Tested?

Compiled docs and manual testing

Screenshots (if appropriate):

Before:
alt text

After:
alt text

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@DaVukovic
Copy link
Member

Good work 👍

Would it make sense to rename "Python API changes" to simply "Python API" and add another sub-section "Python API additions/changs" and have "Python API removed function" as another sub-section from "Python API"?

If that would make sense, I can do that an you could cherrypick.

@MartijnKaijser
Copy link
Member

Personally i don't see the need to split removed functions from the other changes. Are we also going to do separate page for new functions? Changed function?

@DaVukovic
Copy link
Member

DaVukovic commented Oct 12, 2018

@MartijnKaijser

That´s what I´ve suggested above.

AFAICS the current "Python API changes" contains additions and changes of functions. We could include removed functions in there as well. But, tbh, I´m not sure, if that might look confusing then. So my suggestion was to have a structure like that:

Revisisions
- Python API
--Python API function changes/additions
--Python APi removed functions
- Skinning enginge changes

Maybe we should do a similar structure for the skinning enging as well (meaning separating changes/additions from removed fnctions/options).

@MartijnKaijser
Copy link
Member

I was saying it in a way that i don't like splitting it up. Imo a single page for the changes is enough

@DaVukovic
Copy link
Member

ok, fine. I won´t argue if we don´t want that ;)

Was just a suggestion

@MartijnKaijser
Copy link
Member

A changed function is some/most cases as broken as a removed function and they both don't work.

@enen92
Copy link
Member Author

enen92 commented Oct 12, 2018

@MartijnKaijser the goal is this PR is precisely the opposite: create a placeholder page so removed functions show up correctly in the changelog. The goal is not to separate but fix our existing information. I also don't like much the idea of "splitting" removed functions from changes. In fact I spent a bit of time yesterday figuring out how to hide the page from the sidebar. As I was unable to do so, I placed the page under the python revisions section instead of having a new category on the sidebar. If you (or anyone else) know how to hide it from the sidebar please let me know cause that was my primary intention.

@MartijnKaijser
Copy link
Member

MartijnKaijser commented Oct 12, 2018

As i see it this PR has two goals:

  1. Show the removed functions correctly again
  2. Split them into a separate page

I can agree with showing them correctly but not putting them on a special page. There must be a way to put them on the same revisions page as the rest of the v17 changes.

@enen92
Copy link
Member Author

enen92 commented Oct 12, 2018

The problem is that if you don't have the method definition the method documentation will never show correctly in the changelog. And you also won't be able to see the documentation as well. So basically to fix this there are two options:

1 - Create boilerplate methods which return void just for doxygen to work
2 - Move the documentation of the removed function to a new page

I decided to go with 2) (with the idea of "hiding" the page with the only link being available from the changelog) as 1) would reintroduce code just for the sake of having documentation. I don't see any issue with the chosen approach if the page can be hidden from the sidebar.

@enen92 enen92 added the WIP PR that is still being worked on label Oct 12, 2018
@enen92
Copy link
Member Author

enen92 commented Oct 12, 2018

I'll come up with an alternative.

@enen92 enen92 changed the title [docs] Introduce Python removed functions doxygen page [docs] Properly document Python API removed functions in changelog Oct 12, 2018
@enen92 enen92 added the No Jenkins do not run automatic Jenkins builds on this PR label Oct 12, 2018
@enen92
Copy link
Member Author

enen92 commented Oct 12, 2018

@MartijnKaijser @DaVukovic mind taking a second look at the new approach. PR description updated to reflect what was done.

Affected pages and a "live-preview" can be seen below:

https://codedocs.xyz/enen92/xbmc/python_v17.html
https://codedocs.xyz/enen92/xbmc/group__python__xbmc___render_capture.html

As opposed to what we have in master:
https://codedocs.xyz/xbmc/xbmc/python_v17.html
https://codedocs.xyz/xbmc/xbmc/group__python__xbmc___render_capture.html

Regards

@enen92 enen92 removed the WIP PR that is still being worked on label Oct 12, 2018
@DaVukovic
Copy link
Member

That looks good. yes.

@MartijnKaijser
Copy link
Member

MartijnKaijser commented Oct 13, 2018

There seems to be something wrong if you look at https://codedocs.xyz/xbmc/xbmc/group__python__xbmc___render_capture.html

The "bool" is quite misplaced which isn't in the original one.

image

@enen92
Copy link
Member Author

enen92 commented Oct 13, 2018

Sorry I dont get what you mean. That page is the current master and the issues you point are addressed in this PR (see the link with enen92/xbmc instead)

@MartijnKaijser
Copy link
Member

MartijnKaijser commented Oct 13, 2018

doh, you're right. Need coffe

@enen92
Copy link
Member Author

enen92 commented Oct 16, 2018

anything to change @MartijnKaijser ?

@MartijnKaijser MartijnKaijser added this to the Leia 18.0-beta4 milestone Oct 16, 2018
@MartijnKaijser MartijnKaijser merged commit cbf8628 into xbmc:master Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation No Jenkins do not run automatic Jenkins builds on this PR v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants