refactor(sozo): change priority order for arguments

[lambda-0x]

fix: #1336

to be consistent with what we did for torii: #800, i think we should follow this priority, wdyt?

passed cli argument
Environment variable
Scarb.toml 's dojo.tool.env

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e599968) 65.95% compared to head (5027705) 67.13%.
Report is 6 commits behind head on main.

Files	Patch %	Lines
crates/sozo/src/commands/options/account.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1358      +/-   ##
==========================================
+ Coverage   65.95%   67.13%   +1.17%     
==========================================
  Files         228      231       +3     
  Lines       19841    20858    +1017     
==========================================
+ Hits        13087    14003     +916     
- Misses       6754     6855     +101     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[glihm]

fix: #1336

to be consistent with what we did for torii: #800, i think we should follow this priority, wdyt?

passed cli argument
Environment variable
Scarb.toml 's dojo.tool.env

✔️ Agree on this priority list.

1. CLI is explicit so the user do want something specific.
2. Env var as they are very common and offer great flexibility.
3. The scarb manifest allows a simple and quick way to alter those variables.

[kariy]

I initially did cli arg -> scarb.toml -> env var under the assumption that Scarb.toml is perceivably more explicit that env. As user might get confused if they accidentally set both env and Scarb.toml, and have to wonder why the values are different than their Scarb.toml, which is more visually apparent for them then setting an env var.

But I think having env before Scarb.toml might make sense especially when you wanna quickly set those values without having to open a file and edit/delete them manually.

I'm open on changing the priority order if it make sense from user perspective.

[glihm]

I initially did cli arg -> scarb.toml -> env var under the assumption that Scarb.toml is perceivably more explicit that env. As user might get confused if they accidentally set both env and Scarb.toml, and have to wonder why the values are different than their Scarb.toml, which is more visually apparent for them then setting an env var.

But I think having env before Scarb.toml might make sense especially when you wanna quickly set those values without having to open a file and edit/delete them manually.

I'm open on changing the priority order if it make sense from user perspective.

Also I think env variables are very commonly used to alter the behavior of a program while keeping the configuration private. Which can't be done using the scarb manifest. So having the priority changed can allow one to keep dev/frequent config inside the scarb manifest which automatically configures the project for someone that pulls the code and doesn't have to setup any other thing. And the env takes precedence on CI/deploy where more sensible information should be hidden.

Does this make sense to you too guys?

It could be great to have a poll somewhere or a way to have a feedback from users on that.

[lambda-0x]

Does this make sense to you too guys?

Yeah i think the reasoning makes sense

[tarrencev]

My vote is for cli arg > env > config and i think that is fairly expected

[kariy]

Lgtm.

Do you mind following this up with a PR for adding tests to AccountOptions similar to the one we have for StarknetOptions?

[kariy]

Also I think env variables are very commonly used to alter the behavior of a program while keeping the configuration private. Which can't be done using the scarb manifest. So having the priority changed can allow one to keep dev/frequent config inside the scarb manifest which automatically configures the project for someone that pulls the code and doesn't have to setup any other thing. And the env takes precedence on CI/deploy where more sensible information should be hidden.

Make sense, I agree.

[lambda-0x]

Do you mind following this up with a PR for adding tests to AccountOptions similar to the one we have for StarknetOptions?

Sure.

I had a suggestion regarding the tests in StarknetOptions, since environment variables are handled by clap, i think we don't specifically need to tests for environment variables in all the cases.

we can just have one tests which verifies that environment variables are been read, and for all other tests we can pass the arguments directly to Command::parse_from function.

wdyt?

[lambda-0x]

though we can do that change in follow up PR along with addition of tests in AccountOptions

[kariy]

Sure.

Thanks!

I had a suggestion regarding the tests in StarknetOptions, since environment variables are handled by clap, i think we don't specifically need to tests for environment variables in all the cases.

Yeah, I think as long as we can check that it'll fallback to Scarb.toml then should be good