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

remove brackets, update version, update npm cmd #30

Closed
wants to merge 1 commit into from

Conversation

boneyard93501
Copy link

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

Copy link
Collaborator

@shamsartem shamsartem left a comment

Choose a reason for hiding this comment

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

If you think this part of the README is really bad or misleading we can remove usage and usagestop html comments and write this info ourselves but I think we need to just change version in package.json and that's it, everything else is pretty ok in my opinion as it is

and yeah it's weird -v flag not supported by this framework

@@ -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

@shamsartem shamsartem Jun 20, 2022

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

@shamsartem shamsartem Jun 20, 2022

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

@shamsartem shamsartem Jun 20, 2022

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

$ fluence (--version)
@fluencelabs/cli/0.0.2 linux-x64 node-v16.14.0
$ fluence --version
@fluencelabs/cli/0.1.3 darwin-x64 node-v16.15.1
Copy link
Collaborator

@shamsartem shamsartem Jun 20, 2022

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

@shamsartem
Copy link
Collaborator

I will just close this PR for now. I will update version inside package.json myself and if you think (--version) stuff is too confusing let's open a separate PR and remove tags for auto-generation and replace with a different text

@shamsartem shamsartem closed this Jun 28, 2022
@shamsartem shamsartem deleted the fix-readme branch August 1, 2022 09:46
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.

2 participants