-
Notifications
You must be signed in to change notification settings - Fork 11
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
Added stdlib documentation and updated builtins #39
Conversation
im not sure that's a good idea to use ai generated text as the documentation |
It isn't generated, I used it to improve what I already written. |
Please go through the text using LanguageTool or Grammarly — it contains too many mistakes to review. |
What files? because the stdlib is generated from the code in the amber repo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick review, it seems a lot of work will have to be done in the main repo to make the generated docs more consistent
Co-authored-by: Hubert Jabłoński <[email protected]>
Also I enabled GFM so we can have more MD stuff like breaking line with ending double spaces. |
Co-authored-by: Phoenix Himself <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not include .sh
files as it's a compilation product of the main amber script. We can run .ab
with shebang now #!/usr/bin/env amber
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I sorted alphabetically using amber-lang/amber#470 |
docs/advanced_syntax/builtins.md
Outdated
unsafe ${nameof variable}=12$ | ||
// Which is the same as: | ||
let variable = 12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure this is good practice? even though its for demonstration purposes, people will still use it if you put it in docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was something written by @Ph0enixKM, I think that is the only way to explain nameof
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
docs/stdlib/doc.md
Outdated
## Builtin VS StdLib | ||
|
||
[Builtins](/advanced_syntax/builtins) are method that are included in the Amber compiler and don't need to be imported in the code. | ||
|
||
In contrast, the standard library (stdlib) is a collection of Amber functions that are embedded in every Amber release. Each version of Amber may include changes to the standard library and you need to import these functions in your code. These functions are more advanced and can accept various parameters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i fail to see the point of this paragraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to explain the difference between those, we discussed internally why something should became a builtin or a stdflib so deserve a mention in the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this section doesn't explain when something should be builtin or stdlib. It just says that you don't need to import builtin and stdlib you have to import. It's basic truth and I too don't see much sense in writing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this section is important to explain the difference between the two.
Maybe the wording is not the best one but it's a start.
If you have a suggestion on how to improve it right now I will change it as I already did since the PR started.
```ab | ||
import { download } from "std/http" | ||
import { split, contains } from "std/text" | ||
import { file_exist } from "std/fs" | ||
|
||
unsafe $rm -fr /tmp/amber-git$ | ||
if silent download("https://github.com/amber-lang/amber/archive/refs/heads/master.zip", "/tmp/amber-git.zip") { | ||
unsafe $unzip "/tmp/amber-git.zip" -d /tmp/amber-git$ | ||
|
||
let std = unsafe $/usr/bin/ls "/tmp/amber-git/amber-master/src/std/"$ | ||
let stdlib = split(std, "\n") | ||
|
||
loop v in stdlib { | ||
if (contains(v, ".ab") and file_exist("/tmp/amber-git/amber-master/src/std/{v}")) { | ||
unsafe $amber --docs "/tmp/amber-git/amber-master/src/std/{v}" "./docs/stdlib/doc"$ | ||
echo "\n" | ||
} | ||
} | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its a bad idea to hardcode this. if the reader wants to see the code, they should click on the link and open it in github
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now github doens't highlight amber code so it is better to put on the documentation website.
docs/advanced_syntax/builtins.md
Outdated
## Mv | ||
|
||
If you need to move files you can use the `mv` builtin, requires two `Text` parameters. | ||
|
||
```ab | ||
mv "/tmp/a" "/tmp/b" | ||
``` | ||
|
||
This builtin is `failable`, meaning you can handle errors like this: | ||
```ab | ||
mv "/tmp/a" "/tmp/b" failed { | ||
echo "Error" | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about -r
? is it supported? if it is, how do i specify it? if it is not, how do i move a directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is something we should address in Amber and not in the doc website
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also something that should be thoroughly explained in the documentation
the way i see it, documentation must be mostly self sufficient, leave no confusion in interpretation and have no useless text in it |
You're correct, though this is perhaps the wrong place to take issue with some of this. This PR is primarily a script that generates stdlib docs via the doc comments in the main amber repo, so any issue with the content of the stdlib markdown files here, should be addressed in the main repo and then this one can be updated. |
docs/advanced_syntax/builtins.md
Outdated
unsafe ${nameof variable}=12$ | ||
// Which is the same as: | ||
let variable = 12 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Ref: #34
The english text was reviewed/improved with ChatGPT (not written from that).
The stuff in
/doc
is generated from the stdlib comments so for changes refer to https://github.com/amber-lang/amber