-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve samples #6454
Improve samples #6454
Conversation
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.
Most of this is not really useful.
Please don't just remove dead links. Either leave them as is or update to a new location.
Indeed, all that 404s is not lost 😄 |
@z64 Ah yeah using the waybackmachine for the links is a good idea. @straight-shoota But I don't really see why it's a problem to replace the |
Thank you for the PR, but the code as it is right now is fine. |
Official samples should reflect idiomatic Crystal code and its best pratices. Although disagree with some changes (I'm included), this is a legitimate attempt to improve and bring consistency to samples code. I think we should agree on some basic changes (e.g. unifying |
Using tuples for immutable data is not always correct. In samples all arrays have the same type. An array should be used, not a tuple. A tuple is not an immutable array. |
Unifying code syntax is not important. The language allows for many styles, all are correct. |
And I don't see a problem with
Says who?
And that's perfectly fine. I'd even advice to do so. Updating links to new locations is great. Code changes like |
👍 for constants and |
Hmmmm okay changed it back to |
Also accessing constants is slower than accessing local variables. Please, there's no point to these PRs. |
@asterite So I should revert the constants back to local variables? But at for example the But you are right with the |
That's if you use an array literal inside a loop. Accessing a local variable inside a loop is fine. |
Alright changed it. |
I resolved the conflicts. Is anything left? |
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.
Vielen Dank @r00ster91. That's diligent work!
This:
Revives dead links by updating them and using the WayBackMachine
Improves readability a bit by using
abort "message"
instead ofUses
Time.measure
instead ofTime.now - time
Improves other minor things