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

Improve init #129

Merged
merged 8 commits into from
Feb 27, 2024
Merged

Improve init #129

merged 8 commits into from
Feb 27, 2024

Conversation

lukacan
Copy link
Collaborator

@lukacan lukacan commented Feb 23, 2024

Currently, the user experience during the initialization process is not the best. For example, during rustc +nightly nothing is shown within the console so it looks like nothing is happening, or it is possible to call trdelnik init on an already initialized workspace. This PR contains a lot of changes that I was trying to avoid, however, there were some unnecessary functions and logic which I think I (hopefully) successfully simplified.

Init command:

  • Build - after internal discussion I agree that this command is necessary, as If the project cannot be built It can result in incorrectly generated test files. However as we only support anchor-type projects, instead of calling cargo build-sbf, this is changed to anchor build (we only support anchor-type projects)
  • Programs Expansion - The program Expansion function within the Commander was modified to include also very simple progress bar that indicates that something is happening.
    In terms of time, I still think that the rustc +nightly eats most of the time because:
    I tried to benchmark the parse_to_idl_program function which resulted in approximately 25 ms
    Moreover for example fuzz add performs also rustc +nightly with parse_to_idl_program, and this command is almost instant on the already initialized workspace.
  • Folders initialization
    Both and PoC test types - check if the program_client folder structure already exists, if so we expect that the program_client is already initialized = skip initializing program client. Check if PoC folder structure already exists if so skip initializing PoC Tests. For Fuzz Tests logic is the same
    Fuzz test types - this test type is now generated without using program_client. We check if the fuzz_tests folder structure is present if so - skip.
  • Trdelnik.toml and Updating gitignore
    This is the last step

Build Command

  • Checks if Trdelnik.toml is present if so, generate/update program_client. If POC tests are not present (only fuzz tests are present) this will still generate program_client as I found it useful to let the user generate this crate if they want.

Test Command for PoC Tests:

  • This command calls the build command and then test

Fuzz Add

  • Calls the build command so fuzz test files are correctly generated

Other commands are unchanged

Then there are small changes
I updated templates
I moved most of the constants into one private mod
The snake case program name is in snake case

TODO: add into supported version that SOLANA CLI 1.18 is required to use trdelnik init

@lukacan lukacan requested a review from Ikrk February 23, 2024 14:55
@lukacan lukacan changed the title Improve init Improve init + Feb 23, 2024
@lukacan lukacan changed the title Improve init + Improve init Feb 23, 2024
Copy link
Contributor

@Ikrk Ikrk left a comment

Choose a reason for hiding this comment

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

It looks quite good, but again - it is a monster PR. In my opinion it was not absolutelly necessary to change most of the stuff you changed. I tried to review all changes, but I can hardly guarantee there will be no issues.

Also besides my comments, pls add also at least basic documentation to each new function/method.

Regarding: "TODO: add into supported version that SOLANA CLI 1.18 is required to use trdelnik init" - Don't write todos, Just do it! :-) Btw, is it because of Anchor 0.29.0 that cannot be used without Solana 1.18? I actually removed it from readme, because you could not run anchor anyway. But yeah, we might add it as we now use anchor build behind the scenes...

crates/client/src/commander.rs Show resolved Hide resolved
crates/client/src/commander.rs Outdated Show resolved Hide resolved
crates/cli/src/command/test.rs Outdated Show resolved Hide resolved
crates/client/src/test_generator.rs Outdated Show resolved Hide resolved
crates/client/src/test_generator.rs Outdated Show resolved Hide resolved
crates/client/src/test_generator.rs Outdated Show resolved Hide resolved
crates/client/src/test_generator.rs Outdated Show resolved Hide resolved
@lukacan
Copy link
Collaborator Author

lukacan commented Feb 26, 2024

Regarding: "TODO: add into supported version that SOLANA CLI 1.18 is required to use trdelnik init" - Don't write todos, Just do it! :-) Btw, is it because of Anchor 0.29.0 that cannot be used without Solana 1.18? I actually removed it from readme, because you could not run anchor anyway. But yeah, we might add it as we now use anchor build behind the scenes...

Yes I think it is because of anchor 0.29.0 as it uses a dependency that needs rustc version higher than the one which is present with Solana CLI 1.17.*.

Copy link
Contributor

@Ikrk Ikrk left a comment

Choose a reason for hiding this comment

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

When I wrote basic docs, I meant basic docs ;-) Especially private methods do not need extensive docs.

Please implement the include_str!(concat!( (see my comment above) and we can merge...

@lukacan lukacan requested a review from Ikrk February 27, 2024 22:08
🐛 snake case + clone

✅ update tests
➕ dependency for progress bar

⬆️ update locks and tomls for poc examples
🔥 remove unnecessary code

♻️ group dependencies
…ly anchor programs for both test types + add build before poc test
⏪️ root option with Commander

♻️ remove nested block

♻️ spelling

♻️ remove unnecessary condition

♻️ change fn name

📝 add basic documentation for test_generator functions

📝 Commander simple documentation
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.

2 participants