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: factory cleanup #2523

Merged
merged 1 commit into from
Mar 12, 2024
Merged

chore: factory cleanup #2523

merged 1 commit into from
Mar 12, 2024

Conversation

gabrielmer
Copy link
Contributor

@gabrielmer gabrielmer commented Mar 11, 2024

Description

Cleaning node_factory.nim. Only exporting setupNode() and startNode().

Changes

  • moving retrieveDynamicBootstrapNodes() to app.nim
  • not exporting utility procs in node_factory.nim

Issue

extends #2441

@gabrielmer gabrielmer self-assigned this Mar 11, 2024
@gabrielmer gabrielmer requested review from Ivansete-status, SionoiS and NagyZoltanPeter and removed request for SionoiS March 11, 2024 15:45
Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2523

Built from 390cd05

Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Less public procs 💪

Thank you!

@@ -85,6 +85,38 @@ func node*(app: App): WakuNode =
func version*(app: App): string =
app.version

## Retrieve dynamic bootstrap nodes (DNS discovery)

proc retrieveDynamicBootstrapNodes*(dnsDiscovery: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this would be useful to many kind of apps but then without a plugin system of some sort... 🤷 maybe one day!

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.

LGTM! In the future, we may need to revert the mentioned proc movement.
Cheers

@@ -85,6 +85,38 @@ func node*(app: App): WakuNode =
func version*(app: App): string =
app.version

## Retrieve dynamic bootstrap nodes (DNS discovery)

proc retrieveDynamicBootstrapNodes*(dnsDiscovery: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think is better to keep this in the node_factory.nim so that the libwaku could potentially benefit from it when we add this feature. wdyt @jm-clius ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me :) Will wait a bit before merging so we confirm it and if so I'll update the PR 🤩

@gabrielmer gabrielmer merged commit 8d7eb3a into master Mar 12, 2024
11 of 12 checks passed
@gabrielmer gabrielmer deleted the chore-factory-refactor branch March 12, 2024 13:44
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.

3 participants