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

Edit description of invalid array access #2454

Conversation

Amjad50
Copy link
Contributor

@Amjad50 Amjad50 commented Sep 10, 2020

Fix the description in response to a comment from the previous PR #2446 .

fixes #2417

Can you review? @Lonami, @steveklabnik

src/ch03-02-data-types.md Outdated Show resolved Hide resolved
src/ch03-02-data-types.md Outdated Show resolved Hide resolved
@Amjad50 Amjad50 force-pushed the ch3.2-fix-invalid-array-access-description branch from 77dced6 to 7ed5071 Compare September 10, 2020 07:06
Copy link

@Lonami Lonami left a comment

Choose a reason for hiding this comment

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

Looks good to me but I'm pretty sure my wording can be improved. I'll let that job to other reviewers.

src/ch03-02-data-types.md Outdated Show resolved Hide resolved
@Amjad50 Amjad50 force-pushed the ch3.2-fix-invalid-array-access-description branch from 7ed5071 to c19fe2d Compare September 10, 2020 07:15
Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

I think I'm okay with this (once the extra bolding is removed) but would like @carols10cents 's opinion too.

src/ch03-02-data-types.md Outdated Show resolved Hide resolved
@Amjad50 Amjad50 force-pushed the ch3.2-fix-invalid-array-access-description branch from c19fe2d to 97e2b16 Compare September 10, 2020 13:40
@Amjad50
Copy link
Contributor Author

Amjad50 commented Sep 30, 2020

@carols10cents can you please review this PR?

@ibraheemdev
Copy link
Member

Shouldn't it say

Building this code using cargo build produces the following result

Instead of

Running this code using cargo run produces the following result:

Since the point of the example is to show compile-time safety?

@ibraheemdev
Copy link
Member

This issue has caused numerous issues and questions over the past couple months. Can we follow through with the review process?

@Amjad50
Copy link
Contributor Author

Amjad50 commented Dec 21, 2020

Yes of course, thanks for the suggestion

@Amjad50 Amjad50 force-pushed the ch3.2-fix-invalid-array-access-description branch from 97e2b16 to a45c24b Compare December 21, 2020 03:48
@carols10cents
Copy link
Member

@Amjad50 thank you so much for working on this! However, the point of this example is to show the runtime error and demonstrate that there are some safety checks that happen at runtime rather than compile time. I'd rather change the example to something along the lines of this example but not quite that... I'm not sure what yet though, I need some time to think about it.

In the meantime, I'm going to close this PR because I don't think this is the direction we should go.

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.

Chapter 3.2: Example "Invalid Array Element Access" is incorrect
5 participants