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

Add an example with automatic array size #522

Merged
merged 7 commits into from
Apr 15, 2024

Conversation

adriancuadrado
Copy link
Contributor

You can learn more about this here: https://go.dev/blog/slices-intro#arrays

image

@adriancuadrado adriancuadrado changed the title Add am example with automatic array size Add an example with automatic array size Apr 14, 2024
Copy link
Collaborator

@eliben eliben left a comment

Choose a reason for hiding this comment

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

Nice, thank you! Some comments before we proceed

fmt.Println("dcl:", b)

// If you specify the index with `:`, the elements in
// between will be zeroed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is a good place to add that the array length is the maximum index plus one

Copy link
Contributor Author

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 information would add much value since gobyexample is meant for people who already have experience in other programming languages and the length of a list is always the maximum index plus one everywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I mean it specifically in the context of the [...] syntax with explicitly specifying indeces. For example in your second b with the 3:, the array length is 5 because of that.

I don't feel strongly about this, though.

examples/arrays/arrays.go Outdated Show resolved Hide resolved
@adriancuadrado
Copy link
Contributor Author

I've noticed a file called arrays.hash. What is that for? Do I also have to update it?

@eliben
Copy link
Collaborator

eliben commented Apr 15, 2024

I've noticed a file called arrays.hash. What is that for? Do I also have to update it?

You don't need to do it manually, you should run tools/build which will also generate the public HTML for you. Did you do that for this PR?

@adriancuadrado
Copy link
Contributor Author

adriancuadrado commented Apr 15, 2024

@eliben

I've noticed a file called arrays.hash. What is that for? Do I also have to update it?

You don't need to do it manually, you should run tools/build which will also generate the public HTML for you. Did you do that for this PR?

I assumed these kind of things were made automatically by a CI/CD pipeline of some sort, as most people do. I'll run the script and commit the result, but I think you should consider creating a Github Action workflow or something instead.


EDIT: Hold on, you actually have that, but you use it to test the result and make sure I ran the script. Why though? Can't you just use the result of running that script automatically instead?

@eliben
Copy link
Collaborator

eliben commented Apr 15, 2024

I assumed these kind of things were made automatically by a CI/CD pipeline of some sort, as most people do. I'll run the script and commit the result, but I think you should consider creating a Github Action workflow or something instead.

EDIT: Hold on, you actually have that, but you use it to test the result and make sure I ran the script. Why though? Can't you just use the result of running that script automatically instead?

The output in public is currently committed to the repository, because the deployment process is a simple upload to a bucket. I want to keep the CI process simple - don't want it to change PRs/commits - it's only there to verify that everything was done correctly. Running tools/build locally is important for testing the output anyway, so this isn't a problem in reality

@eliben eliben merged commit 7958694 into mmcgrana:master Apr 15, 2024
2 checks passed
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.

None yet

2 participants