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

chore(docs): fix docs and mark some as deprecated #1754

Merged
merged 5 commits into from
May 25, 2023

Conversation

vpavlin
Copy link
Member

@vpavlin vpavlin commented May 23, 2023

Description

First of all, sorry for a lot of changes in a single PR, but once I started...

I went through the docs/ folder and fixed links to repo(s) and CI. I also fixed the doc factually in a few cases and marked some with TODO: Depricate or fix to get your opinion since they are no longer useful/working

Changes

  • fix CI links: ci.status... -> ci.infra.status...
  • fir repo links status-im/nim-waku -> waku-ord/nwaku (no entirely necessary as Github will redirect, but it looks better from contributor/user perspective if the doc is not obviously outdated:) )
  • fix pointers to files (some moved with recent PR(s), som got renamed...)
  • add filter and lightpush v2 examples to waku.nimble and unify the example README

@vpavlin vpavlin requested review from jm-clius, alrevuelta and Ivansete-status and removed request for jm-clius May 23, 2023 12:30
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

This is a very thorough and useful doc review. Thanks!

@@ -20,8 +20,7 @@ and configured to bridge toy-chat messages to the `#waku channel` on the Vac Dis

### Fleet deployment rationale

The `test` fleet is automatically updated after every commit to the `nim-waku` `master` branch
and is therefore the most up to date representation of Waku v2 development.
The `test` fleet is automatically updated after every commit to the `nim-waku` repository `master` branch and is therefore the most up to date representation of Waku v2 development.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `test` fleet is automatically updated after every commit to the `nim-waku` repository `master` branch and is therefore the most up to date representation of Waku v2 development.
The `test` fleet is automatically updated after every commit to the `nwaku` repository `master` branch and is therefore the most up to date representation of Waku v2 development.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, nim-waku is all over the place. I would say we can run the *.md files through sed 's/nim-waku/nwaku/g in a follow up PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, actually we cannot do a simple sed as it would completely mess up references to CI and docker images:D

I fixed the references in the waku-fleets.md and checked a few other files, but seems that it was mainly the fleet doc which was incorrectly referencing nim-waku

Comment on lines 3 to 4
> TODO: Fix or depricate

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion here, but perhaps we can just add the date and a note to say that this refers to an older version of nwaku and direct people to the operator guide? I think we may want to keep the record of old testnets and tutorials, though I'm open to just deleting these from the repo as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the date, but kept the TODO. We can do something about those later

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

That's great, thanks for it! Just a few comments

docs/contributors/continuous-integration.md Outdated Show resolved Hide resolved
Comment on lines 20 to +22
## Interactively add a node

There is also an interactive mode. Type `/connect` then paste address of other node. However, this currently has some timing issues with mesh not being updated, so it is adviced not to use this until this has been addressed. See https://github.com/status-im/nim-waku/issues/231
There is also an interactive mode. Type `/connect` then paste address of other node. However, this currently has some timing issues with mesh not being updated, so it is adviced not to use this until this has been addressed. See https://github.com/waku-org/nwaku/issues/231
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we could remove this section as the issue #231 is closed. In that issue it is said that we already have 'staticnode' option.

Maybe it worth opening a separate issue to clean the /connect option if it doesn't work as expected and won't be used. So maybe we could add another TODO: deprecate or fix ;P

wdyt @jm-clius ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added TODO, thanks:)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Thanks! I don't think we have to spend too much time maintaining docs for previous testnets, although it would be a good idea to indicate that this relates to a (past) testnet for a previous version of nwaku. That said, afaik this issue has been fixed though I haven't used it in a while to confirm. :)

examples/v2/README.md Outdated Show resolved Hide resolved
vpavlin and others added 4 commits May 24, 2023 09:09
Co-authored-by: Ivan Folgueira Bande <128452529+Ivansete-status@users.noreply.github.com>
Co-authored-by: Ivan Folgueira Bande <128452529+Ivansete-status@users.noreply.github.com>
@vpavlin vpavlin merged commit b51fb61 into master May 25, 2023
15 checks passed
@vpavlin vpavlin deleted the feat/update-release-doc branch May 25, 2023 11:37
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

4 participants