-
Notifications
You must be signed in to change notification settings - Fork 677
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/standardized sample dir #5594
base: develop
Are you sure you want to change the base?
Conversation
i can certainly give it a shot
…On Wed, Dec 18, 2024 at 17:25 Jeff Bencin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In clarity/src/vm/docs/mod.rs
<#5594 (comment)>
:
> @@ -3135,7 +3135,7 @@ mod test {
fn test_examples() {
// Execute test examples against the latest version of Clarity
let apis = make_all_api_reference();
- let token_contract_content = include_str!("../../../../sample-contracts/tokens.clar");
+ let token_contract_content = include_str!("../../../../sample/contracts/tokens.clar");
I know you didn't introduce this here, but can we do something cleaner and
less fragile than all these dots and slashes? There is no Cargo environment
variable which contains the workspace root, but you can get it like this
<https://stackoverflow.com/a/74942075>. How about we wrap this in a
LazyLock in stacks-common/util?
—
Reply to this email directly, view it on GitHub
<#5594 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVXIHEYHYIC3C54DHARNQD2GIN7PAVCNFSM6AAAAABT3VBPUOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMJTGE2DINBZGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, this directory layout makes a lot more sense. Just two comments:
- Cargo doesn't provide one, but I think we should provide a
CARGO_WORKSPACE_ROOT
variable as I mentioned in a comment above, instead of using relative paths with dots and slashes - IMO since the config files are required for stackslib unit tests, they should live in stackslib and sample/conf should be a symlink. I don't really feel strongly about this, so I'll approve anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great 🚀
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Further changes involving cargo_workspace()
can be in a separate PR
ref #5575
Creates a new root directoy,
./samples
, and:./testnet/stacks-node/conf
to./sample/conf
./sample-contracts
to./sample/contracts
I've updated all references to the old locations, and re-ran unit-tests and
simple_publish
andsimple_token_transfer
.Note: i've changed the current symlink so the source is in
./sample/conf
, and the target is now./stackslib/conf
(basically, the opposite of what is currently linked) - unit test pass after this change.