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

feat: propagate doc comments on flags and arguments to --help/-h + structs derive refactor #26

Draft
wants to merge 56 commits into
base: master
Choose a base branch
from

Conversation

dj8yfo
Copy link
Contributor

@dj8yfo dj8yfo commented Dec 12, 2024

the crux of the pr is 0c3ce3a
and 912744d

the rest is some refactoring and splitting big chunks of code into smaller and nested modules/functions


the changes to the logic of derives aren't many and are well summarised by
the following list of commits with snapshot differences
from the same tests as added to master branch in
https://github.com/dj8yfo/interactive-clap/commits/backporting_tests_into_master/ branch:


second (or 3rd) sub-summary mentions 2 commits 382cc33, 3db17e1 which made
a testing step automatic and not requiring to manually copy-paste a fragment
from snapshot generated with first test assertion.
No snapshots changed as result of these latter 2 commits.

@dj8yfo
Copy link
Contributor Author

dj8yfo commented Dec 12, 2024

introspect feature can be used to debug how derive proc macros work similar to the following:

# interactive-clap-derive folder
cargo test test_doc_comments_propagate --features introspect -- --nocapture 
# from repo root
cargo run --example struct_with_subargs --features interactive-clap-derive/introspect 

@dj8yfo dj8yfo marked this pull request as ready for review December 13, 2024 18:17
@frol frol requested a review from FroVolod December 22, 2024 19:29
@dj8yfo dj8yfo marked this pull request as draft January 6, 2025 20:07
@dj8yfo
Copy link
Contributor Author

dj8yfo commented Jan 6, 2025

this is still draft, pending some small work on top of it

@dj8yfo dj8yfo force-pushed the propagate_doc_comments_on_flags branch from a6406da to 1d16e9b Compare January 6, 2025 20:30
@dj8yfo dj8yfo mentioned this pull request Jan 9, 2025
@dj8yfo dj8yfo force-pushed the propagate_doc_comments_on_flags branch from 2008066 to f1ad05f Compare January 24, 2025 18:09
@dj8yfo dj8yfo force-pushed the propagate_doc_comments_on_flags branch from b1e5faa to 7c296a4 Compare February 4, 2025 20:39
@dj8yfo
Copy link
Contributor Author

dj8yfo commented Feb 6, 2025

@race-of-sloths include

@race-of-sloths
Copy link

@dj8yfo Thank you for your contribution! Your pull request is now a part of the Race of Sloths!
Do you want to apply for monthly streak? Get 8+ score for a single PR this month and receive boost for race-of-sloths!

Shows inviting banner with latest news.

Shows profile picture for the author of the PR

Current status: waiting for scoring

We're waiting for maintainer to score this pull request with @race-of-sloths score [0,1,2,3,5,8,13] command. Alternatively, autoscoring [1,2] will be applied for this pull request

What is the Race of Sloths

Race of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow

For contributors:

  • Tag @race-of-sloths inside your pull requests
  • Wait for the maintainer to review and score your pull request
  • Check out your position in the Leaderboard
  • Keep weekly and monthly streaks to reach higher positions
  • Boast your contributions with a dynamic picture of your Profile

For maintainers:

  • Score pull requests that participate in the Race of Sloths and receive a reward
  • Engage contributors with fair scoring and fast responses so they keep their streaks
  • Promote the Race to the point where the Race starts promoting you
  • Grow the community of your contributors

Feel free to check our website for additional details!

Bot commands
  • For contributors
    • Include a PR: @race-of-sloths include to enter the Race with your PR
  • For maintainers:
    • Invite contributor @race-of-sloths invite to invite the contributor to participate in a race or include it, if it's already a runner.
    • Assign points: @race-of-sloths score [1/2/3/5/8/13] to award points based on your assessment.
    • Reject this PR: @race-of-sloths exclude to send this PR back to the drawing board.
    • Exclude repo: @race-of-sloths pause to stop bot activity in this repo until @race-of-sloths unpause command is called

Copy link
Member

@akorchyn akorchyn left a comment

Choose a reason for hiding this comment

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

I'll return back to it a bit later. Hard to keep up :)

interactive-clap-derive/src/debug.rs Outdated Show resolved Hide resolved
if attr.path.is_ident("strum_discriminants") {
for attr_token in attr.tokens.clone() {
if let proc_macro2::TokenTree::Group(group) = attr_token {
if &group.stream().to_string() == "derive(EnumMessage, EnumIter)" {
let group_stream_no_whitespace =
group.stream().to_string().replace(" ", "");
Copy link
Member

Choose a reason for hiding this comment

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

maybe split_whitespaces and then collect, or something like that.
maybe some weirdo uses tabs or something like that and ignores fmt.

use proc_macro2::{Span, TokenStream};
use proc_macro_error::abort_call_site;
use quote::{quote, ToTokens};
use syn;

use crate::LONG_VEC_MUTLIPLE_OPT;
#[cfg(test)]
Copy link
Member

Choose a reason for hiding this comment

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

I think cfg test module should be at the end of the file

}
}
/// these are common methods, reused for both the [structs] and `enums` derives
pub(super) mod common_methods;
Copy link
Member

Choose a reason for hiding this comment

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

should be at the top of the file

}

#[doc = include_str!("../../../docs/structs_to_cli_trait_docstring.md")]
pub(crate) mod to_cli_trait;
Copy link
Member

Choose a reason for hiding this comment

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

Same here, move them to the top.

Copy link
Member

Choose a reason for hiding this comment

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

Can you move the docs back to this file (or to the file itself)? I think it's crucial to keep it here, as it's mostly for internal devs, and it's not pleasant to search for a separate file.

As a developer, I never build docs for my crate just to understand the logic.

This is pretty important point for me

Copy link
Contributor Author

@dj8yfo dj8yfo Feb 7, 2025

Choose a reason for hiding this comment

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

yeap, ok
the thing is really convoluted to understand, that's why the doc.
as a reference point to return later to later, when context will be forgotten.
if it helps me navigate, maybe it'll help someone else, though the latter is unlikely

@@ -0,0 +1,27 @@
derive of helper enum for structs with `#[interactive_clap(named_arg)]` on fields
Copy link
Member

Choose a reason for hiding this comment

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

It's internal doc, so I would better keep it with the code at the level with the code

fields
.iter()
.find_map(|field| field_transform(name, field))
.unwrap_or(quote!())
Copy link
Member

Choose a reason for hiding this comment

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

I know it was before, but should we put here that debug macro? to indicate that we ccouldn't transform?

Copy link
Contributor Author

@dj8yfo dj8yfo Feb 10, 2025

Choose a reason for hiding this comment

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

i don't quite understand the suggestion.
if the dbg_cond! macro is implied, i only added it to the paths i had trouble with figuring out for adding tests/looking at a bug or just generally figuring them out.
there are many more possible paths to cover with debug statements, i think there're at least 100x more of them.

Copy link
Contributor Author

@dj8yfo dj8yfo Feb 10, 2025

Choose a reason for hiding this comment

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

i believe this module was a product of extracting this chunk of code
https://github.com/near-cli-rs/interactive-clap/pull/26/files#diff-f36bdc4e97c2bde45b6fdb5764c7a15faf2ab71c0fcd30d123dac570724594c0L147-L192 .
(Github UI doesn't seem to be capable of autoexpanding this link to the exact chunk, you would have to scroll to interactive-clap-derive/src/derives/interactive_clap/mod.rs file and click Load file button to be able to see the exact chunk, without clicking any other links or buttons, or Github will overwrite the info about chunk in URL).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added link to the chunk in the pr's base branch https://github.com/near-cli-rs/interactive-clap/blob/master/interactive-clap-derive/src/derives/interactive_clap/mod.rs#L147-L192 , this appears to work straightforwardly.

@@ -0,0 +1,26 @@
use proc_macro2::TokenStream;
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can just clap_parser_trait_adapter.rs

we don't have any submodules

@akorchyn
Copy link
Member

akorchyn commented Feb 7, 2025

Can you create a patch for near-cli-rs and check that it's starting speed is sufficient and not struggle as Vlad described on a call?

@dj8yfo
Copy link
Contributor Author

dj8yfo commented Feb 7, 2025

@akorchyn patch was created near/near-cli-rs#444 , the starting lag is (visually) the same as in main

Co-authored-by: Artur Yurii Korchynskyi <[email protected]>
@dj8yfo dj8yfo marked this pull request as draft February 10, 2025 15:33
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