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

Make pop new <contract | parachain> consistent #100

Merged
merged 7 commits into from
Apr 5, 2024
Merged

Conversation

weezy20
Copy link
Contributor

@weezy20 weezy20 commented Apr 4, 2024

new parachain does not take any -p, --path arguments but new contract does.
We should stick to consistent messaging formats across both.

cd into {project_path} and enjoy hacking should be the default, and paths should be printed only if the user invoked them with -p, --path

Another inconsistency I noticed was given an arbitrary path, a/b/c to new parachain it would create folders if not present, the same is not done by create_smart_contract. This is now resolved and the corresponding test case has been removed as it's not needed anymore.

@weezy20 weezy20 force-pushed the w/pop-new-contract branch from cf66af8 to 6876460 Compare April 4, 2024 06:39
@weezy20 weezy20 changed the title Consistent message format for pop new contract Consistent message format across pop new contract | parachain Apr 4, 2024
@weezy20 weezy20 force-pushed the w/pop-new-contract branch from c3287b8 to 595a475 Compare April 4, 2024 07:09
@weezy20 weezy20 changed the title Consistent message format across pop new contract | parachain Make pop new <contract | parachain> consistent Apr 4, 2024
@weezy20 weezy20 force-pushed the w/pop-new-contract branch from 595a475 to e6af62a Compare April 4, 2024 07:18
@weezy20 weezy20 requested a review from AlexD10S April 4, 2024 07:38
@brunopgalvao
Copy link
Contributor

Anything need to be updated in the README?

Copy link
Contributor

@brunopgalvao brunopgalvao left a comment

Choose a reason for hiding this comment

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

Makes a lot of sense.

Good job 👏

@weezy20
Copy link
Contributor Author

weezy20 commented Apr 5, 2024

Anything need to be updated in the README?

I don't think so.

@brunopgalvao
Copy link
Contributor

brunopgalvao commented Apr 5, 2024

Anything need to be updated in the README?

I don't think so.

In the {Contracts section](https://github.com/r0gue-io/pop-cli?tab=readme-ov-file#contracts) I would change the instruction so that once pop new contract is called, then cd into the contract directory and run following commands from that perspective e.g. pop build contract, pop test contract. Then mention that you can also run: pop build contract -p ... as an alternative. Same for pop parachain commands.

src/commands/new/parachain.rs Show resolved Hide resolved
src/engines/contract_engine.rs Show resolved Hide resolved
src/engines/contract_engine.rs Show resolved Hide resolved
src/engines/contract_engine.rs Show resolved Hide resolved
@weezy20
Copy link
Contributor Author

weezy20 commented Apr 5, 2024

In the {Contracts section](https://github.com/r0gue-io/pop-cli?tab=readme-ov-file#contracts) I would change the instruction so that once pop new contract is called, then cd into the contract directory and run following commands from that perspective e.g. pop build contract, pop test contract. Then mention that you can also run: pop build contract -p ... as an alternative. Same for pop parachain commands.

the cd instruction with the full contract path is printed after new contract exits. As for build & test, yes pop <build | test> contract may be executed from within the project (parachcain or smart contract) dir, in addition to the existing -p, --path args.

One upcoming feature is PopDb which will allow you to execute pop build from anywhere (if not inside the project already) and build your recently generated pop project. #92

@weezy20 weezy20 merged commit a34cccf into main Apr 5, 2024
1 check passed
@weezy20 weezy20 deleted the w/pop-new-contract branch April 5, 2024 10:35
@AlexD10S AlexD10S mentioned this pull request May 8, 2024
6 tasks
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