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.

Sign up for GitHub

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

Jump to bottom

remove brackets, update version, update npm cmd #30

Closed
wants to merge 1 commit into from
Closed

remove brackets, update version, update npm cmd #30

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ Don't name arguments or flags with names that contain underscore symbols, becaus

<!-- usage -->
```sh-session
$ npm install -g @fluencelabs/cli
$ npm install -location=global @fluencelabs/cli
Copy link
Collaborator

Choose a reason for hiding this comment

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

all this text is auto-generated so it won't be good to change it like that. But I still wanna know why you wanted to change this line. npm i -g something seems so to be the basic way of installing global npm dependencies. Doesn't it work for you? I asked guys with mac-s to check and I think it worked for them

Copy link
Author

Choose a reason for hiding this comment

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

auto-generation is all good if it's correct. clearly

fluence (--version)
zsh: unknown file attribute: v```

is not correct and shouldn't be propagated.

npm -g install @fluencelabs/aqua
npm WARN config global `--global`, `--local` are deprecated. Use `--location=global` instead.

no point propagating deprecated ways

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mentioned this in another comment

fluence (--version) is not correct. And I totally agree it looks confusing. But it's not intended to be executed literally like that
fluence (--version) basically means you can use either fluence --version or just execute fluence without passing any additional flags to see the version and the help message

And about the warning. I didn't see such warning ever before. I searched just now and I see that -g flag is possibly not actually deprecated after all

latest npm doc says npm install -g is the way to go https://docs.npmjs.com/downloading-and-installing-packages-globally and --location flag nowhere to be seen here https://docs.npmjs.com/cli/v8/commands/npm-install

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anatoly found the real reason. So they deprecated it by mistake npm/cli#4982

$ fluence COMMAND
running command...
$ fluence (--version)
@fluencelabs/cli/0.0.2 linux-x64 node-v16.14.0
$ fluence --version
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think creators of the framework decided this auto-generated text makes sense because you can also just say fluence (without using --version flag) and it will show you the help text, so in my personal opinion it's ok to leave it as it is

@fluencelabs/cli/0.1.3 darwin-x64 node-v16.15.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

this one is auto-generated as well as the others (you can spot html comments usage and usagestop and similar - everything between these tags is auto-generated. We can remove the tags if we want to but we need to decide on that)

so this exact line is just an example. It depends on the machine that you use. I see
@fluencelabs/cli/0.1.2 linux-x64 node-v16.15.0

and the version number should be changed in package.json - that's where we should change it instead, I totally forgot to keep doing that

$ fluence --help [COMMAND]
USAGE
$ fluence COMMAND
Expand Down