You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/08/25 17:45:12 UTC
[GitHub] [incubator-nuttx] adamfeuer opened a new pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
adamfeuer opened a new pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647
## Summary
* add a copy of INVIOLABLES.txt converted to rst to the new docs
## Impact
* Two copies of the INVIOLABLES will be present: `INVIOLABLES.txt` and `Documentation/introduction/inviolables.rst`.
## Details
* It seems important to have INVIOLABLES in the new doc system. I tried to convert it to Markdown and include it that way. However, Sphinx toctree can't load documents outside the Documents/ tree, and using include directives also didn't work. Using a symlink did work, but it doesn't seem like such a good idea to have a symlink in version control.
* I'm submitting this here so we can have a discussion about whether this approach is ok for now.
## Testing
* Built the docs manually.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680273923
It's fine what happened is you added the new package but the old package for handling markdown was still there. I should have caught that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680265542
@v01d Reopen button seems to be activated again... strike that, I'll reopen this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476761842
##########
File path: Documentation/Pipfile
##########
@@ -6,10 +6,9 @@ verify_ssl = true
[dev-packages]
[packages]
-recommonmark = "*"
-m2r2 = "==0.2.5"
-Sphinx = "==3.2.1"
-sphinx-rtd-theme = "==0.5.0"
Review comment:
What if we drop the Pipfile and leave the Pipfile.lock. This is a supported workflow for pipenv where it will then just auto create the Pipfile based on the requirements.txt there is even the `--ignore-pipfile` argument which will only use the lock file and not the Pipenv
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680267009
> BTW I'm changing the Pipfile back to having "*" for its dependencies; the Pipfile.lock will hold the specific version that is used to do the last build by someone. That way we can easily check that new versions are working ok, while locking CI and everyday usage to specific known-good versions.
I really don't know much about pyenv, but what I would like to do is to ensure both CI and users building locally use the exact same versions. Sphinx recently fixed something that affected building the documentation. Also, I want to avoid any compatibility issues if any of the python packages change versions. We can always periodically update dependencies, but it is best to stick with known working versions.
If what you suggest still keeps this working (note that requirements.txt is used by CI), you can go ahead with this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680179373
>
>
> I really do not think it is worth duplicating, there's no good reason to do so. You can include the file from Sphinx easily. We discussed converting it to Markdown which is fairly trivial. This way it looks good on Sphinx without any kind of tags and we have a single file.
If there is only one copy that can be in one place, then it MUST be in the top-level directory. It must be unavoidable and not hidden deep in some documentation directory.
It is not a maintenance issue (since it should not change*), so I don't see any problem with two copies either.
Although I hoped some will fix the outdated URL and I think the correct word in line 123 is suit, not suite.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680212518
@adamfeuer if you want you can make a separate PR converting INVIOLABLES file to Markdown and then use `mdinclude` directive to include it somewhere (under contributing? or maybe in the intro).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680176525
I think the duplication is okay. If puts a copy in the Documentation where it makes sense. But keeping the top-level INVIOLABLES.txt is very important too. I am very glad you did not remove it. I want it to be in the top-level directory where is is visable to all can cannot be overlooked. Everyone should working on this project should skim that .txt file; it should be in the most in-yer-face location possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680254628
@v01d Whoops, didn't see your previous comment. We came to the same solution. This PR does what you suggested. If it's ok, let me know, I'll resolve the conflicts and rebase.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680201354
I have no attachment to the action :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680265223
@v01d Ok, it seems like force-pushing this messed something up and closed the PR. I'm going to open a new one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680258312
@v01d Yes, putting it directly in the `toctree` failed for me, so I think we need to do it this way.
BTW I'm changing the Pipfile back to having "*" for its dependencies; the Pipfile.lock will hold the specific version that is used to do the last build by someone. That way we can easily check that new versions are working ok, while locking CI and everyday usage to specific known-good versions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680176525
I think the duplication is okay. If puts a copy in the Documentation where it makes sense. But keeping the top-level INVIOLABLES.txt is very important too. I am very glad you did not remove it. I want it to be in the top-level directory where is is visiable to all can cannot be overlooked. Everyone should working on this project should skim that .txt file; it should be in the most in-yer-face location possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on a change in pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476817478
##########
File path: Documentation/Pipfile
##########
@@ -6,10 +6,9 @@ verify_ssl = true
[dev-packages]
[packages]
-recommonmark = "*"
-m2r2 = "==0.2.5"
-Sphinx = "==3.2.1"
-sphinx-rtd-theme = "==0.5.0"
Review comment:
@v01d I'll keep them simple and make them match.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680293863
@v01d Re: the Markdown formatting in `INVIOLABLES.md` – it has to be formatted that way, since in Markdown, heading 1 uses `=` and heading 2 uses `-`, and there are no other heading styles like that; heading 3 needs to be `###`. That's not the case in RST, which has many different heading styles.
If we don't do it this way, the left-hand side tree menu generated by Sphinx will not work correctly.
Alternatively, we can format the file with `#` for heading 1 and `##` for heading 2, but that seemed to get further away from the way the file was originally formatted.
If you have a better idea, let me know.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476746549
##########
File path: Documentation/Pipfile
##########
@@ -6,10 +6,9 @@ verify_ssl = true
[dev-packages]
[packages]
-recommonmark = "*"
-m2r2 = "==0.2.5"
-Sphinx = "==3.2.1"
-sphinx-rtd-theme = "==0.5.0"
Review comment:
We should be supplying minimum working versions for these. I'm sure there are versions that will not work.
I would rather just tell people using pipenv to just use pipenv install -r requirements.txt instead of maintaining things in two places. But I understand you want a fully pinned requirement list.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680268933
The commit message has a really long list of unrelated commits. Would you mind editing that so that it only describes this commit? Sorry for the hassle. Rebasing sometimes is painful.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680184842
@v01d If you can get it included from the top level directory, that would be fantastic. I could not do it.
Here's a couple of links that I saw in my research:
* [StackOverflow on including files outside document root](https://stackoverflow.com/questions/10199233/can-sphinx-link-to-documents-that-are-not-located-in-directories-below-the-root) – there is a workaround for `.rst` files but not for `.md` files
* https://github.com/sphinx-doc/sphinx/issues/701 – very old but still active discussion about relative links in `toctree` directive
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476803604
##########
File path: Documentation/Pipfile
##########
@@ -6,10 +6,9 @@ verify_ssl = true
[dev-packages]
[packages]
-recommonmark = "*"
-m2r2 = "==0.2.5"
-Sphinx = "==3.2.1"
-sphinx-rtd-theme = "==0.5.0"
Review comment:
My only expectation would be that both CI and people who clone the repo and tries to build the docs will use the same version of these packages and they will not be updated until someone decides to do so. If we stick to a version that works, I don't see how that could break. There's also not much reason to upgrade frequently.
Anyway, I do not feel strongly about either, I don't have python experience to justify any preference. I don't like duplication in general and requirements.txt seems easier to me to maintain (as I don't use pyenv either). But I'll go with whatever you both feel is best.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on a change in pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476790828
##########
File path: Documentation/Pipfile
##########
@@ -6,10 +6,9 @@ verify_ssl = true
[dev-packages]
[packages]
-recommonmark = "*"
-m2r2 = "==0.2.5"
-Sphinx = "==3.2.1"
-sphinx-rtd-theme = "==0.5.0"
Review comment:
@btashton @v01d The problem with using just a requirements.txt file is that you have to manually update requirements, test, and pin them because you're trying to use one file for both functions; and you have no tooling to support you. In my experience this makes the project brittle. The requirements don't get updated often and the project breaks eventually.
pipenv solves this by having a workflow that supports pinned requirements for CI and regular users, while developers can update the Pipfile.lock easily using pipenv's automatic pinning. I would prefer we keep them and do this instead whenever requirements are updated:
pipenv run pip freeze > requirements.txt
That said, if both of you feel strongly that we should delete the Pipfile/Pipfile.lock I'll go along with it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680254108
@v01d Ok, I figured how to do what you suggested. the `m2r` python module is not being maintained and doesn't work for `mdincludes` above the doc root. But [`m2r2`](https://github.com/CrossNox/m2r2) is a fork that does work.
@patacongo This PR now converts the `INVIOLABLES.txt` to a Markdown file, there is a slight change in formatting to how the Enemies subpoints are marked as subheadings, will you see if this is ok with you?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680302571
@v01d Ok I made all headings use the `#` formatting.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680191737
@patacongo Ok, fixed the suit typo and URL to coding standard in both files.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476748830
##########
File path: Documentation/Pipfile
##########
@@ -6,10 +6,9 @@ verify_ssl = true
[dev-packages]
[packages]
-recommonmark = "*"
-m2r2 = "==0.2.5"
-Sphinx = "==3.2.1"
-sphinx-rtd-theme = "==0.5.0"
Review comment:
Using requirements.txt was also my preference, but again I'm not a python expert
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680199179
The docs build with a makefile so we can add logic there to setup the build environment like moving files around. I agree that it should stay at the top and not be duplicated. We will have to address this anyway for things like the board information.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680272896
@btashton can you quickly take a look? As you requested I update the Pipfile in the other PR and this one partly overrides that I would like to confirm this is OK for everyone.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680200220
> The docs build with a makefile so we can add logic there to setup the build environment like moving files around. I agree that it should stay at the top and not be duplicated. We will have to address this anyway for things like the board information.
I thought about the same but GH actions builds running sphinx-build directly so that would not work. Maybe we can run make on Documentation on GH actions if that helps later on.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680181661
> > I really do not think it is worth duplicating, there's no good reason to do so. You can include the file from Sphinx easily. We discussed converting it to Markdown which is fairly trivial. This way it looks good on Sphinx without any kind of tags and we have a single file.
>
> If there is only one copy that can be in one place, then it MUST be in the top-level directory. It must be unavoidable and not hidden deep in some documentation directory.
>
> It is not a maintenance issue (since it should not change*), so I don't see any problem with two copies either.
>
> Although I hoped some will fix the outdated URL at line 59 and I think the correct word in line 123 is suit, not suite.
I understand it might not change but I think there's no good reason to duplicate it. Yes, the idea is to leave it in top-level directory.
I can do this later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680308663
@v01d @btashton I squashed again and force pushed, so this should be good to merge now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer edited a comment on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
adamfeuer edited a comment on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680293863
@v01d Re: the Markdown formatting in `INVIOLABLES.md` – it has to be formatted that way if you want to use underline style headers, since in Markdown, heading 1 uses `=` and heading 2 uses `-`, and there are no other heading styles like that; heading 3 needs to be `###`. That's not the case in RST, which has many different heading styles.
If we don't do it this way, the left-hand side tree menu generated by Sphinx will not work correctly.
Alternatively, we can format the file with `#` for heading 1 and `##` for heading 2, but that seemed to get further away from the way the file was originally formatted.
If you have a better idea, let me know.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680297231
> Alternatively, we can format the file with `#` for heading 1 and `##` for heading 2, but that seemed to get further away from the way the file was originally formatted.
This is the usual approach with Markdown (the one with underlines is considered "alternative"), all other md files already use this style as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680257090
You can try rebasing. Right now the only thing remaining would be conversion to Markdown and inclusion in docs. BTW, if you're going to have a standalone section for the inviolables, you can simply list it in the toctree (unless that fails, in which case you can do as you did).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680178323
I really do not think it is worth duplicating, there's no good reason to do so. You can include the file from Sphinx easily. We discussed converting it to Markdown which is fairly trivial. This way it looks good on Sphinx without any kind of tags and we have a single file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton merged pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
btashton merged pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on a change in pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476812318
##########
File path: Documentation/Pipfile
##########
@@ -6,10 +6,9 @@ verify_ssl = true
[dev-packages]
[packages]
-recommonmark = "*"
-m2r2 = "==0.2.5"
-Sphinx = "==3.2.1"
-sphinx-rtd-theme = "==0.5.0"
Review comment:
@v01d Ok, I would prefer to use the pipenv flow then. Re: requirements not changing... well I've been developing python in commercial environments for a long time. Every time my teams have frozen requirements and hoped they won't need to change, that bit us. Software on the Internet is bit like sand on the beach, it looks static but actually moves quite a bit over time.
In another PR I'll write the README.md that explains how to maintain the Pipenv, Pipfile.lock, and requirements.txt then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476815160
##########
File path: Documentation/Pipfile
##########
@@ -6,10 +6,9 @@ verify_ssl = true
[dev-packages]
[packages]
-recommonmark = "*"
-m2r2 = "==0.2.5"
-Sphinx = "==3.2.1"
-sphinx-rtd-theme = "==0.5.0"
Review comment:
Ok, cool.
Remember that some of this information is already on the contributing -> documentation itself. Let's try to keep those with the same instructions. I'm thinking mostly about someone who just looks at the documentation and has not yet even cloned the NuttX repo.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on a change in pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476649190
##########
File path: Documentation/index.rst
##########
@@ -11,6 +11,7 @@
NuttX Documentation
===================
+
Review comment:
@patacongo Oops will fix
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680179373
>
>
> I really do not think it is worth duplicating, there's no good reason to do so. You can include the file from Sphinx easily. We discussed converting it to Markdown which is fairly trivial. This way it looks good on Sphinx without any kind of tags and we have a single file.
If there is only one copy that can be in one place, then it MUST be in the top-level directory. It must be unavoidable and not hidden deep in some documentation directory.
It is not a maintenance issue (since it should not change), so I don't see any problem with two copies either.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680309288
Ok, LGTM to me. I'll let @btashton merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680276476
Just looked again at the changes and it looks like the INVIOLABLES file is strangely formatted now, it uses "=" and "-" for some headings and "#" prefix for others (it should use the latter).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680176525
I think the duplication is okay. If puts a copy in the Documentation where it makes sense. But keeping the top-level INVIOLABLES.txt is very important too. I am very glad you did not remove it. I want it to be in the top-level directory where is is visiable to all can cannot be overlooked. Everyone should working on this project should skim that .txt file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680193685
I'll look into it. It is possible that including an external markdown is not possible as you mentioned.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680187105
One easy way is to include it verbatim. I did this with the license in licensing.rst:
<pre>
.. include:: ../../LICENSE
:literal:
</pre>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer closed pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
adamfeuer closed pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1647: convert INVIOLABLES.txt to markdown and add to docs
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476735213
##########
File path: Documentation/conf.py
##########
@@ -115,3 +115,5 @@
'CODE',
'noreturn_function'
]
+
+source_suffix = ['.rst', '.md']
Review comment:
This is already there
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] adamfeuer commented on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
adamfeuer commented on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680192642
@v01d Yeah I guess doing it with literal works. It would be nice to have a formatted version though. Is there another way you know to do this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#issuecomment-680179373
>
>
> I really do not think it is worth duplicating, there's no good reason to do so. You can include the file from Sphinx easily. We discussed converting it to Markdown which is fairly trivial. This way it looks good on Sphinx without any kind of tags and we have a single file.
If there is only one copy that can be in one place, then it MUST be in the top-level directory. It must be unavoidable and not hidden deep in some documentation directory.
It is not a maintenance issue (since it should not change*), so I don't see any problem with two copies either.
Although I hoped some will fix the outdated URL at line 59 and I think the correct word in line 123 is suit, not suite.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1647: add a copy of INVIOLABLES.txt converted to rst to the new docs
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1647:
URL: https://github.com/apache/incubator-nuttx/pull/1647#discussion_r476648787
##########
File path: Documentation/index.rst
##########
@@ -11,6 +11,7 @@
NuttX Documentation
===================
+
Review comment:
Double spaced? Is that consistent?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org