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

feat(cli): added #45

Merged
merged 4 commits into from
Sep 25, 2024
Merged

feat(cli): added #45

merged 4 commits into from
Sep 25, 2024

Conversation

Mte90
Copy link
Member

@Mte90 Mte90 commented Sep 12, 2024

ref #34

@Mte90 Mte90 requested a review from Ph0enixKM September 12, 2024 08:50
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

I don't think that creating a new file just for CLI usage is necessary. We can move the subsections from the cli.md to the usage guide

Comment on lines 3 to 17
```
Usage: amber [OPTIONS] [INPUT] [OUTPUT]

Arguments:
[INPUT] '-' to read from stdin
[OUTPUT] '-' to output to stdout, '--silent' to discard

Options:
-e, --eval <EVAL> Code to evaluate
--docs Generate docs (OUTPUT is dir instead, default: `docs/`)
--disable-format Don't format the output file
--minify Minify the resulting code
-h, --help Print help
-V, --version Print version
```
Copy link
Member

Choose a reason for hiding this comment

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

This help message will change pretty frequently. I think that we shouldn't include it here

Copy link
Member Author

Choose a reason for hiding this comment

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

I was including as reference so people can understand what are the parameters, the man pages usually have this kind of output.
We can update after a release if there are changes.

Copy link
Member

Choose a reason for hiding this comment

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

Can we write somewhere that this is just a reference? Also what do you think about it @b1ek @KrosFire @mks-h

Copy link
Member Author

Choose a reason for hiding this comment

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

we can put on top "as per version 0.3.5 this is the help output"

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if people will use it and if we will always remember to update this. This is auto generated and I think that standard human behaviour is to use -h flag when one doesn't know how to use the cli

Copy link
Member

Choose a reason for hiding this comment

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

i think we should just tell people to install the cli and use --help. this will be misleading especially if the user has a different version installed

Copy link
Member Author

Choose a reason for hiding this comment

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

the documentation is for the actual version and not the development release.
at every release the plan is to update the documentation so that section will be updated too, right now I added the version where it was generated.
think that any documentation as this output shown as reference, in this way you don't have to install the tool.

docs/getting_started/cli.md Outdated Show resolved Hide resolved
docs/getting_started/cli.md Outdated Show resolved Hide resolved
docs/getting_started/usage.md Outdated Show resolved Hide resolved
Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

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

lgtm for now, but it should be worked on later

@Mte90 Mte90 merged commit 6c386a3 into amber-lang:master Sep 25, 2024
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.

4 participants