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

FCP HIPE: Indy documentation framework #63

Merged
merged 14 commits into from Dec 13, 2018
Merged

FCP HIPE: Indy documentation framework #63

merged 14 commits into from Dec 13, 2018

Conversation

michaeldboyd
Copy link
Contributor

@michaeldboyd michaeldboyd commented Nov 28, 2018 •
edited

@michaeldboyd
added indy-docs-framework HIPE
19e50c5 
Signed-off-by: michael <michael.boyd@sovrin.org>
@michaeldboyd michaeldboyd changed the title propose HIPE: Sphinx and Readthedocs multi-repo documentation framework propose HIPE: Indy multi-repo documentation framework Nov 28, 2018
@michaeldboyd michaeldboyd changed the title propose HIPE: Indy multi-repo documentation framework propose HIPE: Indy documentation framework Nov 29, 2018
@michaeldboyd
added formatting and wording changes from proofreading
2556f88 
Signed-off-by: michael <michael.boyd@sovrin.org>
@michaeldboyd
removed indy-docs-guidelines duplicate
1037ac9 
Signed-off-by: michael <michael.boyd@sovrin.org>
@pknowl
Copy link

pknowl commented Nov 30, 2018

Beautiful work, Michael! I look forward to adding some documentation form the # indy-semantics WG over the coming weeks/months. A fabulous start.

kdenhartog
kdenhartog approved these changes Nov 30, 2018
View reviewed changes
Copy link
Contributor

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

This looks awesome! With regards to the .rst and .md question I find it beneficial to support both.

dhh1128
dhh1128 reviewed Nov 30, 2018
View reviewed changes
text/indy-docs-framework/README.md Outdated Show resolved Hide resolved
text/indy-docs-framework/README.md Outdated
- indy-node: https://github.com/hyperledger/indy-node
- indy-agent: https://github.com/hyperledger/indy-agent
- indy-plenum: https://github.com/hyperledger/indy-plenum
- indy-hipe: https://github.com/hyperledger/indy-hipe
Copy link
Member

Choose a reason for hiding this comment

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

If you are proposing to manage HIPE documents differently, this PR will need to include changes to the root README.md in indy-hipe, because it describes doc management conventions that differ from what you advocate here.

Copy link
Member

Choose a reason for hiding this comment

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

Note also that the choices made about indy-hipe will also need to ripple to the github.com/sovrin-foundation/sovrin-sip repo, as it is a fork. And we will need many in-process HIPEs to be modified to meet the new conventions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a note to the HIPE here about how the indy-hipe will need to change a couple minor things in order to be built with Sphinx and Readthedoc:

@michaeldboyd
Copy link
Contributor Author

I propose we move this to FCP, unless there is any other feedback on the current draft.

@kdenhartog
Copy link
Contributor

kdenhartog commented Dec 6, 2018 via email

@nage nage changed the title propose HIPE: Indy documentation framework FCP HIPE: Indy documentation framework Dec 6, 2018
@michaeldboyd
changed HIPE directory to reflect new number
d2da78a 
Signed-off-by: michael <michael.boyd@sovrin.org>
@dhh1128
Copy link
Member

dhh1128 commented Dec 7, 2018 •
edited

I am not feeling good about the proposal to enforce one <h1> per HIPE doc. There are many HIPEs where that will be problematic. You can't just indent headers one more level, as many of them use <h1>, <h3>, <h5>, and <h7> today. Maybe we should talk about that?

@swcurran
Copy link
Member

swcurran commented Dec 7, 2018 •
edited

I am not feeling good about the proposal to enforce one <h1> per HIPE doc. There are many HIPEs where that will be problematic. You can't just indent headers one more level, as many of them use <h1>, <h3>, <h5>, and <h7> today. Maybe we should talk about that?

Based on @michaeldboyd's work - this is a constraint of RTDs and I think we are more than happy with the benefits we get from that vs. this constraint. If this can be resolved within the construct of RTDs, without creating multiple pages per HIPE (which I think is doubtful), I'm good with that, but if not - we just live with it.

@swcurran
Copy link
Member

swcurran commented Dec 7, 2018

The thing I'm not seeing in this is the use of sphinx/RTDs to extract and publish the external interface (API/Methods, etc.) documentation from directly code. That's where we started with RTDs and it's been super valuable. (see https://von-anchor.readthedocs.io/en/latest/modules.html).

Presumably, this would be another HIPE, but I think we would want this mentioned as a goal - that we strive to make developers aware of the protocols for inline external interface code documentation that can be extracted and published using RTD. This is a huge help for the users of a library such as indy-sdk - providing not only useful documentation of the interface for a library, but also links to the associated code. Powerful stuff!

swcurran
swcurran approved these changes Dec 7, 2018
View reviewed changes
Copy link
Member

@swcurran swcurran left a comment

Choose a reason for hiding this comment

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

Looks great! Looking forward to having it fully implemented.

text/0025-indy-docs-framework/README.md Show resolved Hide resolved
text/0025-indy-docs-framework/README.md Outdated Show resolved Hide resolved
text/0025-indy-docs-framework/README.md Show resolved Hide resolved
@michaeldboyd
Copy link
Contributor Author

michaeldboyd commented Dec 7, 2018 •
edited

@dhh1128

I am not feeling good about the proposal to enforce one <h1> per HIPE doc. There are many HIPEs where that will be problematic. You can't just indent headers one more level, as many of them use <h1>, <h3>, <h5>, and <h7> today. Maybe we should talk about that?

I don't love it either... the only other option I've found would be to edit the parsing code within the RTD recommonmark parsing library, and that would be much more intensive.

I've taken the initiative to fix the current hipes and the template so they only have 1 <h1> header. Here's what it will look like when complete: https://indy.readthedocs.io/projects/hipe/en/latest/index.html

Are even numbered headers off limits for some reason? I can edit the currently proposed HIPEs moving forward. After those are resolved, the template and readme can make sure future HIPEs work by default.

@michaeldboyd
removed mentions of 'I' and replaced with 'we' or second person direc…
00fe172 
…tives. Also included the indy-docs-conf proposal for repositories

Signed-off-by: michael <michael.boyd@sovrin.org>
@michaeldboyd
added suggestions from reviewers to include a .gitignore, to tell doc…
515a20d 
…s creators to think of their audience, and to remove first person references. removed mention of the indy-docs-conf. That repository will become deprecated as the remote_conf file has been moved to the indy-docs repository

Signed-off-by: michael <michael.boyd@sovrin.org>
@michaeldboyd michaeldboyd mentioned this pull request Dec 10, 2018
Closed
@michaeldboyd
Copy link
Contributor Author

How do the maintainers feel about this PR currently?

@michaeldboyd michaeldboyd mentioned this pull request Dec 12, 2018
Closed
@TelegramSam TelegramSam merged commit bb1db4f into hyperledger:master Dec 13, 2018
brentzundel pushed a commit to brentzundel/indy-hipe that referenced this pull request Apr 16, 2019
@TelegramSam
Merge pull request hyperledger#63 from michaeldboyd/master
4537f51 
FCP HIPE: Indy documentation framework

Signed-off-by: Brent <brent.zundel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhh1128 dhh1128 dhh1128 left review comments

@nemqe nemqe nemqe left review comments

@dbluhm dbluhm dbluhm left review comments

@swcurran swcurran swcurran approved these changes

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

None yet
9 participants
@michaeldboyd @pknowl @kdenhartog @dhh1128 @swcurran @nemqe @dbluhm @TelegramSam @nage