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

Integrate go perun tutorial #48

Merged

Conversation

matthiasgeihs
Copy link
Contributor

No description provided.

source/conf.py Show resolved Hide resolved
@choeppler
Copy link
Contributor

The CircleCI build broke because of some invalid external links. You can also run that test with make linkcheck locally.

@ggwpez
Copy link

ggwpez commented Feb 25, 2021

The CI check for the tutorial code is now missing. @matthiasgeihs

@ggwpez
Copy link

ggwpez commented Feb 25, 2021

The CircleCI build broke because of some invalid external links. You can also run that test with make linkcheck locally.

Weird since the link that is reported as broken works fine. Maybe it is inserted after the page-load into the DOM.

@manoranjith
Copy link
Contributor

manoranjith commented Feb 26, 2021

From the CI logs, these were the broken links:

  1. (go-perun/tutorial/channels/updating: line 33) broken disputes.html -

I have added a suggestion with 4 changes to fix this one.

As @ggwpez pointed out, this link is be valid but is reported as broken, probably since it is inserted after page load.

  1. (go-perun/tutorial/getting-started: line 35) broken https://github.com/trufflesuite/ganache-cli#installation - Anchor 'installation' not found

Copy link
Contributor

@manoranjith manoranjith left a comment

Choose a reason for hiding this comment

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

Suggestions to fix broken link. Includes 4 changes.

source/go-perun/tutorial/channels/updating.rst Outdated Show resolved Hide resolved
source/go-perun/tutorial/channels/updating.rst Outdated Show resolved Hide resolved
source/go-perun/tutorial/setup/contracts.rst Show resolved Hide resolved
@matthiasgeihs
Copy link
Contributor Author

The CircleCI build broke because of some invalid external links. You can also run that test with make linkcheck locally.

From the CI logs, these were the broken links:

  1. (go-perun/tutorial/channels/updating: line 33) broken disputes.html -

I have added a suggestion with 4 changes to fix this one.

As @ggwpez pointed out, this link is be valid but is reported as broken, probably since it is inserted after page load.

  1. (go-perun/tutorial/getting-started: line 35) broken https://github.com/trufflesuite/ganache-cli#installation - Anchor 'installation' not found

any idea how to fix this? @manoranjith

@manoranjith
Copy link
Contributor

The CircleCI build broke because of some invalid external links. You can also run that test with make linkcheck locally.

From the CI logs, these were the broken links:

  1. (go-perun/tutorial/channels/updating: line 33) broken disputes.html -

I have added a suggestion with 4 changes to fix this one.

As @ggwpez pointed out, this link is be valid but is reported as broken, probably since it is inserted after page load.

  1. (go-perun/tutorial/getting-started: line 35) broken https://github.com/trufflesuite/ganache-cli#installation - Anchor 'installation' not found

any idea how to fix this? @manoranjith

For the first one, have made a suggestion, for the second one I haven't found a fix yet, will have to still look for it.

In case, if we are not able to find any fix for it,y suggestion would be make linkcheck as a separate CI job and make it optional. The reviewer will then have to decide whether the Pr can be merged in cases where the job fails.

@choeppler
Copy link
Contributor

In case, if we are not able to find any fix for it,y suggestion would be make linkcheck as a separate CI job and make it optional. The reviewer will then have to decide whether the Pr can be merged in cases where the job fails.

I dimly remember that there's a timeout option for the linkecheck. I think I once saw some other docs projects where that was increased. Could that be a solution for the problem?

@manoranjith
Copy link
Contributor

manoranjith commented Feb 26, 2021

In case, if we are not able to find any fix for it,y suggestion would be make linkcheck as a separate CI job and make it optional. The reviewer will then have to decide whether the Pr can be merged in cases where the job fails.

I dimly remember that there's a timeout option for the linkecheck. I think I once saw some other docs projects where that was increased. Could that be a solution for the problem?

I tried the timeout option with 30s. It still was reported as broken link.

Another option is to exclude check of anchors in this. In this case, the validity of link will be checked. This could be enabled by adding the following lines to the end of conf.py file. I found from sphinx docs that this option was specifically targeting cases were anchors were dynamically added to a web page.

@matthiasgeihs @choeppler

linkcheck_ignore = [
   'https://github.com/trufflesuite/ganache-cli#'
]

I couldn't make a suggestion as no lines were modified near the end of the file.

@choeppler
Copy link
Contributor

@manoranjith, now that you've investigated this linkcheck problem and we have an example, could you also file a bug report with the sphinx project to maybe get the root cause solved? Thanks!

Signed-off-by: Matthias Geihs <matthias@perun.network>
Revise title.
Include Perun Logo.
Add more text.

Signed-off-by: Matthias Geihs <matthias@perun.network>
Signed-off-by: Matthias Geihs <matthias@perun.network>
Signed-off-by: Matthias Geihs <matthias@perun.network>
Signed-off-by: Matthias Geihs <matthias@perun.network>
@manoranjith
Copy link
Contributor

@manoranjith, now that you've investigated this linkcheck problem and we have an example, could you also file a bug report with the sphinx project to maybe get the root cause solved? Thanks!

I think this is a known limitation in linkcheck, because they have provided options exclusively to handle these scenarios where anchors in the URL are dynamically added (say by Javascript). However, I can check the existing issues in their repo to see the understand more and will create an issue if there is none covering this problem.

@matthiasgeihs
Copy link
Contributor Author

matthiasgeihs commented Mar 1, 2021

I have addressed all comments, I think. Please have another look.

I externalized the tutorial source code into a separate repository at perun-network, where we can also gather additional code examples. The code is included here via a git submodule. I added submodule initialization to the ci and added a remark to the README.

For some reason LGTM takes ages to run since I added the submodule :(

@ggwpez
Copy link

ggwpez commented Mar 2, 2021

We could also add https://github.com/perun-network/perun-tutorial as a sub-module here instead of copying the files.
But i was not involved in the planning meeting for the integration, so consider this void if it does not make sense.

README.md Outdated Show resolved Hide resolved
source/go-perun/index.rst Show resolved Hide resolved
source/go-perun/tutorial/channels/closing.rst Show resolved Hide resolved
source/index.rst Show resolved Hide resolved
@manoranjith
Copy link
Contributor

manoranjith commented Mar 3, 2021

@matthiasgeihs The PR LGTM now. I have 2 minor comments and a review from @choeppler on the change in conf.py:

  • one is a suggestion to add (in the readme file) the steps for checking out git sub modules

  • other, as a remainder to create follow up issues.

  • Need review comments from @choeppler: changing authors and copyright holder name from Hyperledger to Perun Framework Contributors in conf.py.

The PR can be merged after this.

@manoranjith
Copy link
Contributor

@matthiasgeihs Also could you add a one-line explanation in this commit message a09bfab, the reason for disabling autosection extension. The reason, I understand is to use manually created tags instead.

source/go-perun/tutorial/index.rst Outdated Show resolved Hide resolved
source/conf.py Show resolved Hide resolved
@matthiasgeihs
Copy link
Contributor Author

We could also add https://github.com/perun-network/perun-tutorial as a sub-module here instead of copying the files.
But i was not involved in the planning meeting for the integration, so consider this void if it does not make sense.

I thought about it, but there were several changes necessary, hence I did not follow this approach.

go-perun doc is using manually created section labels. Furthermore there were collisions between the section titles.

Signed-off-by: Matthias Geihs <matthias@perun.network>
Signed-off-by: Matthias Geihs <matthias@perun.network>
Signed-off-by: Matthias Geihs <matthias@perun.network>
Signed-off-by: Matthias Geihs <matthias@perun.network>
@matthiasgeihs matthiasgeihs force-pushed the integrate-go-perun-tutorial branch 3 times, most recently from 567ef3c to b83e0fb Compare March 3, 2021 17:07
Signed-off-by: Matthias Geihs <matthias@perun.network>
Signed-off-by: Matthias Geihs <matthias@perun.network>
Copy link
Contributor

@manoranjith manoranjith left a comment

Choose a reason for hiding this comment

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

LGTM.

@manoranjith manoranjith dismissed choeppler’s stale review March 4, 2021 14:29

The suggested changes have been accepted.

@manoranjith manoranjith merged commit 3ff7776 into hyperledger-labs:master Mar 4, 2021
manoranjith added a commit that referenced this pull request Oct 14, 2021
Resolves #48

Signed-off-by: Manoranjith <ponraj.manoranjitha@in.bosch.com>
@matthiasgeihs matthiasgeihs deleted the integrate-go-perun-tutorial branch June 13, 2022 11:34
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.

None yet

6 participants