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

Improved config feature #198

Merged
merged 36 commits into from
Feb 27, 2024
Merged

Conversation

prxgr4mm3r
Copy link
Collaborator

No description provided.

src/lib/swankyCommand.ts Outdated Show resolved Hide resolved
src/types/index.ts Outdated Show resolved Hide resolved
src/types/index.ts Show resolved Hide resolved
src/lib/swankyCommand.ts Outdated Show resolved Hide resolved
src/lib/command-utils.ts Outdated Show resolved Hide resolved
src/commands/account/default.ts Show resolved Hide resolved
src/commands/contract/deploy.ts Outdated Show resolved Hide resolved
src/commands/contract/deploy.ts Outdated Show resolved Hide resolved
src/commands/init/index.ts Outdated Show resolved Hide resolved
src/lib/swankyCommand.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ipapandinas ipapandinas left a comment

Choose a reason for hiding this comment

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

I've made a series of detailed commits that address various aspects of the logic. When new comments are fixed, I would appreciate it if you could thoroughly retest the entire flow to make sure everything is working correctly.

src/lib/swankyCommand.ts Outdated Show resolved Hide resolved
src/lib/swankyCommand.ts Outdated Show resolved Hide resolved
src/lib/swankyCommand.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Outdated Show resolved Hide resolved
src/lib/contractCall.ts Outdated Show resolved Hide resolved
src/lib/command-utils.ts Outdated Show resolved Hide resolved
src/commands/init/index.ts Show resolved Hide resolved
src/commands/contract/deploy.ts Show resolved Hide resolved
src/lib/swankyCommand.ts Outdated Show resolved Hide resolved
@prxgr4mm3r
Copy link
Collaborator Author

@ipapandinas What do you think? If an account was created outside the project dir, should it be merged into this.swankyConfig from the system config when we call the command from the project dir?

@ipapandinas
Copy link
Contributor

@prxgr4mm3r I do not think so, if it's a personal dev account you do not want to share it publicly.
Here is how I see it:

  • Account created/set outside the project dir: Update Swanky system config only
  • Account created/set in the project dir: Update both Swanky project + system config
  • Account created/set passing an env Swanky config variable: Update Swanky env config only

When setting the default account we can check if the alias belongs to the current account list.

For example:

Scenario 1

  • I created an "hello" dev account outside a project dir => Swanky system config only is updated
  • I navigate to a project dir and I set change "hello" as default => Swanky system config only is updated, the default account in the Swanky local project dir config is unchanged

Scenario 2

  • I created a "bye" dev account inside a project dir => Swanky system config + project dir config are updated
  • I set "bye" as default account => Swanky system config + project dir config are updated

Scenario 3

  • I use account create command with an env Swanky config => Swanky env config only is updated
  • I use account default command with an env Swanky config => Swanky env config only is updated

@prxgr4mm3r
Copy link
Collaborator Author

@ipapandinas What about accounts that are stored in the system config before the project is created? Should it be stored in the local config of the new project?

@ipapandinas
Copy link
Contributor

@prxgr4mm3r good question but I don't think so. System account are personal accounts and project accounts are shared with project contributors.

@ipapandinas
Copy link
Contributor

@prxgr4mm3r I created this commit 3a70313 on another branch to explore the possibility of using a config builder. The goal is to streamline the process of updating the configuration in a safe and controlled manner. What do you think? If you agree, you can cherry pick my commit and go from there.

@prxgr4mm3r
Copy link
Collaborator Author

@ipapandinas I like this idea! Will work on that.

@prxgr4mm3r prxgr4mm3r linked an issue Jan 31, 2024 that may be closed by this pull request
6 tasks
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 left a comment

Choose a reason for hiding this comment

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

greatly improves the experience!

protected async loadAndMergeConfig(): Promise<void> {
try {
const systemConfig = getSwankyConfig("global");
this.swankyConfig = { ...this.swankyConfig, ...systemConfig };
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be like { ...systemConfig, ...this.swankyConfig }? or we are sure that they are disjoint, so it doesn't matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here this.swankyConfig holds the default state set during the buildSwankyConfig init phase. I will rename it to make it clear: this.swankyConfig = { ...swankyDefaultConfig, ...systemConfig };

@@ -6,13 +6,15 @@ export class Query extends ContractCall<typeof Query> {

static args = { ...ContractCall.callArgs };

static flags = { ...ContractCall.callFlags };

public async run(): Promise<void> {
const { flags, args } = await this.parse(Query);
Copy link
Member

Choose a reason for hiding this comment

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

does this destructor shadow the new static flags?

Copy link
Contributor

Choose a reason for hiding this comment

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

After testing it, I've observed that this.parse primarily uses static flags and args. However, I must admit is confusing..


await writeJSON(path.resolve("swanky.config.json"), this.swankyConfig, { spaces: 2 });
const configBuilder = new ConfigBuilder(getSwankyConfig("local"));
configBuilder.addContract(args.contractName);
Copy link
Member

Choose a reason for hiding this comment

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

nit: it might be a bit more convenient to make addContract, addContractDeployment methods more suited for the builder pattern (to return this) - we could then chain all configuration into a single expression

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed! Let me adjust it

@ipapandinas ipapandinas changed the base branch from master to ink-devhub-1 February 26, 2024 13:52
@ipapandinas ipapandinas merged commit e315716 into ink-devhub-1 Feb 27, 2024
3 checks passed
@ipapandinas ipapandinas deleted the feature/update-swanky-config branch February 27, 2024 10:43
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.

Improved Config Feature
3 participants