Skip to content
This repository has been archived by the owner on Oct 4, 2024. It is now read-only.

add yarg’s strict() to chain. add walletUrl, contractName as options #265

Merged
merged 10 commits into from
Mar 31, 2020

Conversation

mikedotexe
Copy link
Contributor

@mikedotexe mikedotexe commented Feb 6, 2020

Different approach to #259
Also enabled recommendations. So near fake will fail, list possible commands, and say, "Did you mean near stake?"
I think the issue was .config(config) loaded src/config.js which has keys that weren't explicitly set as options.

This is quite separate from the PR where most of the discussion on this issue has been happening.
#262

If we choose to merge this PR, we can close the other.

Update: this PR has been languishing and Illia brought up that this is important. I see that @vgrichina suggests we overhaul how config is used. Can we get something done in the meantime?

Tests have been failing and I have commits fixing these, including some outdated shell commands. Kind of mysterious that tests have worked at all. I guess some errant calls technically don't exit(1)? Doesn't matter too much, this is ready for re-review.

@mikedotexe mikedotexe requested a review from vgrichina February 6, 2020 19:20
bin/near-cli.js Outdated Show resolved Hide resolved
@vgrichina
Copy link
Contributor

I think the issue was .config(config) loaded src/config.js which has keys that weren't explicitly set as options.

yes, that was the problem

not sure if it's not confusing to keep these as options for all commands (as most commands would ignore --contractName for example) or also what may happen given that it's alias to --accountId – we might end up using value from src/config.js for commands which don't operate on contracts.

for this reason I feel that long-term we need to figure out solution where we load config in different way

@vgrichina
Copy link
Contributor

@mikedotexe this is failing the tests BTW

@mikedotexe
Copy link
Contributor Author

@mikedotexe this is failing the tests BTW

Yes, I haven't seen any PRs pass tests the past two days.

Unknown argument: masterAccount
++RESULT='{ networkId: '\''shared-test'\'',
  nodeUrl: '\''http://shared-test.nearprotocol.com:3030'\'',
  contractName: undefined,
  masterAccount: '\''test.near'\'' }'
./test/test_generate_key.sh FAIL
error Command failed with exit code 1.

masterAccount is not something I've touched, which is odd.
Locally, when I run rm -rf neardev && yarn test on master it's also failing, but for a different reason.

Account has existing key pair with ed25519:EsyKEPeLbgPk9qorxLZ2thLghLXJXdfMubF8muy5mg2j public key =~ .*Account has existing key pair with ed25519:.+ public key.* ]]
./test/test_generate_key.sh SUCCESS
error Command failed with exit code 1.

I've had to somewhat ignore errors as it seems something is awry.

@mikedotexe
Copy link
Contributor Author

The latest commit reduces that confusion. When a user types near or near --help it does not show the walletUrl or contractName flags.
I hear what you're saying, @vgrichina, about loading the config. I think this approach will work. I also sense there are going to be a significant changes to shell that are coming soonish. This is a fairly quick approach. I feel we want to improve UX the original issue soon, giving users feedback on what went wrong when they type something incorrect.

Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

I think the issue was .config(config) loaded src/config.js which has keys that weren't explicitly set as options.

What does this mean? I don't see any change related to .config(config)

I see eight commits here for a pretty small set of changes. I'd prefer we kept our commit history clean, so that it can be a good reference for us in the future. I don't think it makes sense to have more than one commit in this PR – do you mind squashing them together, either now or when you merge, and making sure the commit has a good description?

See a couple other comments & questions below. I don't currently feel informed enough to mark this as "approved" or as "request changes"

bin/near-cli.js Outdated Show resolved Hide resolved
.option('masterAccount', {
desc: 'Master account used when create new accounts',
type: 'string',
hidden: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why hide all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good debate to be had. The short answer is, there's a long road to getting near-shell consistent, and this is something of a bandaid. Actually, this whole PR is a bandaid.
Note that when you run: near to display the default help from yargs, it spits out commands and then the Options: section. These are flags that typically (again, we're working on it) apply to the top-level commands. Actually, the only good example of this is --accountId here.
Then, when you run near create_account --help you'll see the relevant Options: for that particular command, including --masterAccount.
This hidden key is not hiding it everywhere, but hiding it from the top-level list. I'm not against removing the hidden part, but it'll just make th

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops. It'll just make that a bit longer. The fundamental issue here is we want to use strict mode, but we can't do that when we load config.js and it contains additional flags that aren't captured at the top level. So in this PR we're like, "yeah, you can give me a masterAccount flag here, it won't do much on its own, but you can"
And from there yargs's strict mode is possible whereby it'll scream at you if you misspell something or provide suggestions (recommendCommands) or tell you what you're missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent explanation, thank you

test/test_account_operations.sh Show resolved Hide resolved
test/test_generate_key.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@janedegtiareva janedegtiareva left a comment

Choose a reason for hiding this comment

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

Looks good! Have you tested it on windows? Do you mind commenting on the PR with how you tested it? LGTM after making sure it works on diff platforms

@mikedotexe
Copy link
Contributor Author

Looks good! Have you tested it on windows? Do you mind commenting on the PR with how you tested it? LGTM after making sure it works on diff platforms

On Windows I run near fake and nothing happens. Then when I'm on this branch I can run:
node bin\near fake and it'll suggest a similar, valid command.
2020-03-31 14_39_19-Window

Same thing on OS X, except this time I'll use an errant argument.
near state --fake argie mikedotdmg
makes it look like everything is fine, leading the user to believe they may need this --fake argument.
running on this PR:
./bin/near state --fake argie mikedotdmg
It tells the user that fake isn't a thing.

Screenshot 2020-03-31 14 47 22

The first line of this PR's description links to the issue created by Illia who was trying to run:
near transfer acc1 my_validator 1

This has the same behavior.

@mikedotexe mikedotexe merged commit 2932fb2 into master Mar 31, 2020
@chadoh chadoh deleted the add-config-vars-as-args branch April 1, 2020 00:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants