Skip to content

chore: document NodeConfig parameters #6000

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

Merged
merged 11 commits into from
May 16, 2025

Conversation

Jiloc
Copy link
Contributor

@Jiloc Jiloc commented Apr 8, 2025

Description

Add documentation for all NodeConfig paramters

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@Jiloc Jiloc requested a review from a team as a code owner April 8, 2025 19:00
@Jiloc Jiloc requested a review from wileyj April 8, 2025 19:06
@wileyj
Copy link
Collaborator

wileyj commented Apr 8, 2025

few minor nits, but overall this is a great start!

@obycode obycode self-requested a review April 9, 2025 14:46
@Jiloc Jiloc self-assigned this Apr 11, 2025
@aldur aldur added this to the 3.1.0.0.9 milestone May 2, 2025
@aldur aldur moved this to Status: 💻 In Progress in Stacks Core Eng May 2, 2025
@aldur aldur moved this from Status: 💻 In Progress to Status: In Review in Stacks Core Eng May 2, 2025
@aldur aldur requested a review from obycode May 2, 2025 14:38
@aldur aldur modified the milestones: 3.1.0.0.9, 3.1.0.0.10 May 6, 2025
@aldur aldur requested a review from wileyj May 6, 2025 14:50
Copy link
Contributor

@obycode obycode 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 good, I just had some minor comments.

I think these descriptive comments should also be added to the NodeConfigFile struct, since this would be the one that would be most important to extract into documentation for users. Maybe it is less important to document the default values on NodeConfig and more important to show that on NodeConfigFile, since it would be shown to users trying to understand the config file.

@obycode
Copy link
Contributor

obycode commented May 7, 2025

I think these descriptive comments should also be added to the NodeConfigFile struct, since this would be the one that would be most important to extract into documentation for users. Maybe it is less important to document the default values on NodeConfig and more important to show that on NodeConfigFile, since it would be shown to users trying to understand the config file.

Per discussion in the Nakamoto sync, let's leave the comments as-is for now, and in a separate issue, we will work on merging the two versions of the config structs.

@Jiloc Jiloc requested a review from a team as a code owner May 7, 2025 21:50
@Jiloc
Copy link
Contributor Author

Jiloc commented May 7, 2025

I think these descriptive comments should also be added to the NodeConfigFile struct, since this would be the one that would be most important to extract into documentation for users. Maybe it is less important to document the default values on NodeConfig and more important to show that on NodeConfigFile, since it would be shown to users trying to understand the config file.

Per discussion in the Nakamoto sync, let's leave the comments as-is for now, and in a separate issue, we will work on merging the two versions of the config structs.

@obycode Unfortunately, I couldn't attend today's sync :( , but your first comment is interesting because I actually came to a similar conclusion about where the docs might best live. When I worked on ConnectionOptionsFile in PR #6062, I realized that documenting the *File struct directly made more sense for user-facing TOML documentation, especially since the internal ConnectionOptions struct had tens fields not exposed in the config file that didn't make any sense to document this way.

Given that, I'd be open to move the documentation to the *File structs now, I'm happy to make that change for this PR (and the other 2). It should be a quick refactor on my end. This would give us more user-focused docs in the short term.

Of course, I'm also perfectly fine sticking to the plan from the sync if that's preferred, and then we can address it all during the struct merge.

Either way, great idea merging the two struct versions in the long run. It'll definitely simplify things and make the config handling clearer. We'll just have to decine on a strategy for the internal-only fields, but that's for the future discussion.

@Jiloc
Copy link
Contributor Author

Jiloc commented May 9, 2025

I think these descriptive comments should also be added to the NodeConfigFile struct, since this would be the one that would be most important to extract into documentation for users. Maybe it is less important to document the default values on NodeConfig and more important to show that on NodeConfigFile, since it would be shown to users trying to understand the config file.

Per discussion in the Nakamoto sync, let's leave the comments as-is for now, and in a separate issue, we will work on merging the two versions of the config structs.

@obycode Unfortunately, I couldn't attend today's sync :( , but your first comment is interesting because I actually came to a similar conclusion about where the docs might best live. When I worked on ConnectionOptionsFile in PR #6062, I realized that documenting the *File struct directly made more sense for user-facing TOML documentation, especially since the internal ConnectionOptions struct had tens fields not exposed in the config file that didn't make any sense to document this way.

Given that, I'd be open to move the documentation to the *File structs now, I'm happy to make that change for this PR (and the other 2). It should be a quick refactor on my end. This would give us more user-focused docs in the short term.

Of course, I'm also perfectly fine sticking to the plan from the sync if that's preferred, and then we can address it all during the struct merge.

Either way, great idea merging the two struct versions in the long run. It'll definitely simplify things and make the config handling clearer. We'll just have to decine on a strategy for the internal-only fields, but that's for the future discussion.

As per discussion in today's Naka sync, we'll keep the comments in the "non-file" structs, because the comments will be useful for the developers. We'll try to achieve the merging of the Config structs in the near future.

@aldur aldur requested review from obycode and fdefelici May 13, 2025 14:49
Copy link
Contributor

@fdefelici fdefelici left a comment

Choose a reason for hiding this comment

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

lgmt!

@obycode obycode added this pull request to the merge queue May 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 15, 2025
@Jiloc Jiloc added this pull request to the merge queue May 16, 2025
Merged via the queue into stacks-network:develop with commit eda674c May 16, 2025
210 checks passed
@Jiloc Jiloc deleted the chore/document-node-config branch May 16, 2025 12:07
@github-project-automation github-project-automation bot moved this from Status: In Review to Status: ✅ Done in Stacks Core Eng May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants