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.

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

Adding naming documentation #39

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Adding naming documentation #39

wants to merge 1 commit into from

Conversation

mohanad0mohamed
Copy link
Contributor

Fixes #22

  • Edited conf.py for configuration of colors

  • Edited index.rst to include naming.rst

  • Added naming.rst with info on naming convention.

  • Added extra.css for coloring visuals

  • Built locally with no errors

@mohanad0mohamed mohanad0mohamed requested a review from mithro July 26, 2022 08:35
@mohanad0mohamed
Copy link
Contributor Author

I have found that the last commit on main branch already contains symbolic link to open-source-pdks which contains naming.rst also. But still we will need the changes in:
conf.py
extra.css

@mohanad0mohamed
Copy link
Contributor Author

naming.rst is removed and index.rst is the default with no edits.

@mithro
Copy link
Contributor

mithro commented Jul 26, 2022

We should move the extra.css and the python stuff in conf.py into https://github.com/google/open-source-pdks and then this repository should then just use the files from third_party/open-source-pdks like it does with the rst files.

@mohanad0mohamed
Copy link
Contributor Author

mohanad0mohamed commented Jul 28, 2022 •
edited
Loading

I'm afraid that may not happen, at least for conf.py, as it's used by read-the-docs/sphinx to configure the docs build. The problem is each project may use options different than the other like color scheme, link to their repository, and extensions.
The extra.css can be moved no problems with it.

@mithro mithro requested a review from atorkmabrains August 2, 2022 17:07
atorkmabrains
atorkmabrains approved these changes Aug 2, 2022
View reviewed changes
Copy link
Collaborator

@atorkmabrains atorkmabrains left a comment

Choose a reason for hiding this comment

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

I approve this change per @mithro request to clean up the documentation.

@mithro
Copy link
Contributor

mithro commented Aug 3, 2022

I have rebased this change on top of the other work which has been merged. Will add a review shortly.

@mithro
Copy link
Contributor

mithro commented Aug 3, 2022

@mohanad0mohamed - I'm not sure the title of this pull request doesn't make sense any more?

mithro
mithro suggested changes Aug 3, 2022
View reviewed changes
Copy link
Contributor

@mithro mithro left a comment

Choose a reason for hiding this comment

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

Do you think my suggestions here make sense?

docs/_static/extra.css Outdated Show resolved Hide resolved
docs/conf.py Outdated
@@ -181,5 +181,94 @@
[author], 1)
]


Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this python code into a sphinx extension module.

We can then add the extension to the requirements.txt and the extension list found at https://github.com/google/gf180mcu-pdk/blob/main/docs/conf.py#L51-L62

We can probably store the extension in the repository at https://github.com/google/open-source-pdks

@mohanad0mohamed mohanad0mohamed mentioned this pull request Aug 3, 2022
Merged
1 task
@mohanad0mohamed mohanad0mohamed force-pushed the naming branch 3 times, most recently from 57b06ff to 7361981 Compare August 3, 2022 19:38
@mohanad0mohamed mohanad0mohamed requested a review from mithro August 3, 2022 19:39
@mohanad0mohamed
Copy link
Contributor Author

I have edit the required files to depend on open-source-pdks
This PR google/open-source-pdks#18 should be merged first to take effect.

@mohanad0mohamed mohanad0mohamed mentioned this pull request Aug 3, 2022
Open
mithro
mithro approved these changes Aug 5, 2022
View reviewed changes
Copy link
Contributor

@mithro mithro left a comment

Choose a reason for hiding this comment

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

This pull request looks really good!

Just one minor comment which is actually about google/open-source-pdks#18

docs/conf.py Outdated
@@ -59,6 +60,7 @@
'sphinx.ext.mathjax',
'sphinx.ext.napoleon',
'sphinx.ext.todo',
'ext'
Copy link
Contributor

Choose a reason for hiding this comment

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

We should give this extension a better name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@mohanad0mohamed
Copy link
Contributor Author

@mithro I have fixed the name as requested from ext.py to sphinx_pdk_roles.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atorkmabrains atorkmabrains atorkmabrains approved these changes

@mithro mithro mithro 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.

Add documentation about the naming scheme
3 participants
@mohanad0mohamed @mithro @atorkmabrains