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

fix(zk_toolbox): make ecosystem configuration not found error more clear #2128

Closed
wants to merge 9 commits into from

Conversation

zk-Lumi
Copy link
Contributor

@zk-Lumi zk-Lumi commented Jun 3, 2024

What ❔

  • Make ecosystem configuration not found error more clear

Why ❔

  • User needs to check readme again to make sure they are in the right folder

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.
  • Spellcheck has been run via zk spellcheck.

@zk-Lumi zk-Lumi marked this pull request as ready for review June 3, 2024 10:55
@@ -201,7 +201,7 @@ impl EcosystemConfig {
/// Result of checking if the ecosystem exists.
#[derive(Error, Debug)]
pub enum EcosystemConfigFromFileError {
#[error("Ecosystem configuration not found")]
#[error("Ecosystem configuration not found (make sure you have created one `zk_inception ecosystem create` + are in the ecosystem folder `cd path/to/ecosystem/name`)")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, we could do something:
Could not find 'ZkStack.toml' in <path>, please create ecosystem first zk_inception ecosystem create`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most common I see for this error is when people forget to cd into ecosystem folder after zk_inception ecosystem create. Can you think of alternative wording you would prefer to my wording?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably at the end of the create phase, we could show that you have to cd to the new path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a good idea also but I think most people will overlook that, would be good to have it in the error too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Deniallugo updated wording + added message in ecosystem created, mind reviewing?

@@ -214,7 +214,7 @@ impl EcosystemConfig {
/// Result of checking if the ecosystem exists.
#[derive(Error, Debug)]
pub enum EcosystemConfigFromFileError {
#[error("Ecosystem configuration not found")]
#[error("Ecosystem configuration not found (Could not find 'ZkStack.toml' in <path>: Make sure you have created an ecosystem & are in the new folder `cd path/to/ecosystem/name`)")]
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to print the actual path? or no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense to me - Updated

@Deniallugo Deniallugo closed this Jul 11, 2024
@Deniallugo
Copy link
Contributor

closed in favour of #2424

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants