Skip to content

chore: repo and code upgrades - #2479

Open
igamigo wants to merge 9 commits into
nextfrom
igamigo-repo-upgrades
Open

chore: repo and code upgrades#2479
igamigo wants to merge 9 commits into
nextfrom
igamigo-repo-upgrades

Conversation

@igamigo

@igamigo igamigo commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Closes #2471 and supersedes #2472.

  • cargo xtask fmt-comments: ports feat: xtask for reflowing comments node#2099. Adds tree-sitter comment reflow and disables rustfmt comment wrapping. Reflows a bunch of comments. I'm not entirely sure we want to keep this. Stylistically a few type of changes are not fully better IMO, but there are a bunch of others where it's a clear improvement and it makes things much more consistent. I added a couple changes to the upstream script (mainly leaving note-type comments on their own lines)
  • Rust 1.98: Updates toolchain, MSRV, and docs.
  • AGENTS.md: ports Discourage drive-by AIs node#2539, including ASD-STE100 guidance and no unrequested commits or pushes. Did a couple of tests specifically with this and seems like it's overall better. Here I expanded it to point agents to the contributing guide.
  • Comment cleanup: inspired by Improve comments node#2540, removing comments that restate code or describe historical diffs.

All changes in different commits in case we want to roll something back

@igamigo
igamigo force-pushed the igamigo-repo-upgrades branch from 8e93f2d to ea49be1 Compare September 1, 2026 18:01
@igamigo
igamigo marked this pull request as ready for review September 1, 2026 18:03
@igamigo
igamigo force-pushed the igamigo-repo-upgrades branch 2 times, most recently from 551bc9f to 1e69294 Compare September 2, 2026 14:55
@igamigo
igamigo requested a review from sofiazcoaga September 2, 2026 14:56
@igamigo
igamigo force-pushed the igamigo-repo-upgrades branch from 1e69294 to 340aaa0 Compare September 2, 2026 15:11
// add swap note's tag to client2
// we could technically avoid this step, but for the first iteration of swap notes we'll
// require to manually add tags
// add swap note's tag to client2 we could technically avoid this step, but for the first

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// add swap note's tag to client2 we could technically avoid this step, but for the first
// add swap note's tag to client2, we could technically avoid this step, but for the first


// sync on client 2, we should get the swap note
// consume swap note with accountB, and check that the vault changed appropriately
// sync on client 2, we should get the swap note consume swap note with accountB, and check that

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// sync on client 2, we should get the swap note consume swap note with accountB, and check that
// sync on client 2, we should get the swap note, consume swap note with accountB, and check that

Comment thread crates/sqlite-store/src/note/mod.rs Outdated
// There's no need to retrieve it separately.
// let script_root = row.get(0)?;
// The script root can be derived from the script itself. There's no need to retrieve it
// separately. let script_root = row.get(0)?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the last bit of this doc is actuall disabled code.

Suggested change
// separately. let script_root = row.get(0)?;
// separately.

Comment thread crates/rust-client/src/test_utils/common.rs Outdated
Comment thread crates/rust-client/src/sync/state_sync.rs Outdated
Comment thread bin/miden-cli/src/utils.rs Outdated
Comment thread bin/miden-bench/src/masm.rs Outdated
Comment thread bin/integration-tests/build.rs Outdated
Comment thread bin/integration-tests/src/tests/onchain.rs Outdated
Comment thread crates/rust-client/src/sync/state_sync.rs Outdated
Comment thread Makefile Outdated
.PHONY: format
format: ## Run format using nightly toolchain
format: ## Reflow comments, then run format using nightly toolchain
cargo xtask fmt-comments --write

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

xtask runs before rustfmt, so it computes each comment's width from the indentation the file has before rustfmt settles it. When rustfmt then re-indents that comment, the wrap it already computed no longer matches the new column, and the line could end up over comment_width, causing the format check to fail.

@igamigo igamigo Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, I had the original command ordering based on the node's changes but swapping them avoids this, so I ended up doing that.


let mut words = Vec::new();
for line in block {
let text = safe_comment_text(&line.content)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I'm understanding correctly, the ? here returns from reflow_block, so when one line is rejected by safe_comment_text the whole block is skipped and not only that line. An empty /// line is rejected, so, for example, a doc comment with a summary, an empty line and a body is never reflowed. And with wrap_comments = false nothing else checks comment_width there. Is that intended?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

True as well. Made it so empty comment lines are paragraph boundaries, and also so fenced blocks are left untouched. The paragraph split had exposed doctest code so the diff grew with more reformatting.

I think a structural line still causes the whole paragraph to be skipped intentionally and that's fine, since lists, tables, and code-adjacent comments aren't necessarily text you want to readjust.

@igamigo
igamigo force-pushed the igamigo-repo-upgrades branch from 0bdfd5b to 6390d46 Compare September 3, 2026 17:57
@igamigo
igamigo force-pushed the igamigo-repo-upgrades branch from 6390d46 to 2707a41 Compare September 3, 2026 18:00
@igamigo

igamigo commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

I think all comments got addressed, so this should be ready for another round of reviews

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.

ci: declared MSRV 1.96 is below the locked dependency floor

3 participants