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

Add Lua API docs #4

Merged
merged 13 commits into from
Sep 22, 2020
Merged

Add Lua API docs #4

merged 13 commits into from
Sep 22, 2020

Conversation

twaritwaikar
Copy link

Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

Let me know what you think of my suggestions. A bit of manual revision as well, and it should be good to go :)

pages/lua_manual/api_reference.md Show resolved Hide resolved
pages/lua_manual/api_reference.md Outdated Show resolved Hide resolved
pages/lua_manual/api_reference.md Outdated Show resolved Hide resolved
pages/lua_manual/api_reference.md Outdated Show resolved Hide resolved
pages/lua_manual/api_reference.md Outdated Show resolved Hide resolved
pages/lua_manual/api_reference.md Outdated Show resolved Hide resolved
pages/lua_manual/api_reference.md Outdated Show resolved Hide resolved
@twaritwaikar
Copy link
Author

Alright, the formatting should be okay now. If there are any more changes to that let me know. I will now go through the binding code and try to verify all the docstrings.

@mikke89
Copy link
Owner

mikke89 commented Sep 21, 2020

Looks good to me. The only thing that stands out a bit to me is the long table of contents list. Maybe we could section it a bit better. For example "main types" such as LuaRmlUi, Context, Element. And Utilities: Vector, Color. Special elements: ElementFormControlInput etc. These are just suggestions.

@twaritwaikar
Copy link
Author

Sure that will be nice. I can break the table into those 3 types manually.

@twaritwaikar
Copy link
Author

@mikke89 Sent the first manual review I was able to do. I went over the bindings in the Lua plugin and verified all of them with the docs

@twaritwaikar
Copy link
Author

I will be doing another review over all the docs strings to verify that all of them make sense

@mikke89
Copy link
Owner

mikke89 commented Sep 22, 2020

Nice work, let me know when you feel it's ready to be merged.

By the way, are you still generating the markdown from your script? If so, it would be nice to include the script and json data into the repository so that it is easy to extend and modify the documentation in the future.

@twaritwaikar
Copy link
Author

@mikke89 I have gone over the docstrings once and fixed all the "Python terminology" that I could find (which wasn't translating to Lua as it is very well) and converted them to their Lua counterparts.

It should be okay to merge from my side.

And no, after this commit, I started doing manual changes just because I didn't want to edit the JSON directly since that was a single source of truth I had and the task left was the manual review itself. But anyways, I have now pushed the script and the data in this PR so that it can be used again later if required.

Although I don't see it being used later unless the docs become too out of date with the original API :)

@mikke89
Copy link
Owner

mikke89 commented Sep 22, 2020

Alright, thanks a lot!

I figured the script might be useful when we add or modify functions, or perhaps want to change the layout later. At least now we have it just in case it becomes useful :)

I'll merge this very soon. Do you mind if I put the RmlUi MIT license on the python script?

@twaritwaikar
Copy link
Author

twaritwaikar commented Sep 22, 2020 via email

@mikke89 mikke89 merged commit 0dd2de9 into mikke89:master Sep 22, 2020
@mikke89
Copy link
Owner

mikke89 commented Sep 22, 2020

Thanks again! This has been a long time on the to-do list.

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.

2 participants