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 Appendix A: Keywords to Sway book #4992

Merged
merged 20 commits into from
Aug 29, 2023

Conversation

andrewvious
Copy link
Contributor

Description

closes #4932

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@eureka-cpu eureka-cpu added enhancement New feature or request The Sway Book Everything to do with the Sway Book labels Aug 23, 2023
@eureka-cpu eureka-cpu enabled auto-merge (squash) August 23, 2023 17:16
Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

  1. I have not checked all the links to ensure that they point to where they ought to
  2. Unfortunately, I really do not have the bandwidth to make individual suggestions for each definition but I would re-write most / almost all of the definitions to be more accurate / concise. Most aren't bad but ought to be updated imo

docs/book/src/appendix/keywords.md Outdated Show resolved Hide resolved
docs/book/src/appendix/keywords.md Outdated Show resolved Hide resolved
docs/book/src/appendix/keywords.md Outdated Show resolved Hide resolved
docs/book/src/appendix/keywords.md Outdated Show resolved Hide resolved
@andrewvious
Copy link
Contributor Author

  1. I have not checked all the links to ensure that they point to where they ought to
  2. Unfortunately, I really do not have the bandwidth to make individual suggestions for each definition but I would re-write most / almost all of the definitions to be more accurate / concise. Most aren't bad but ought to be updated imo

Appreciate the review, all the links I found should check out! As far as the definitions I tried to make it as boiled down as possible, using rust's appendix as reference (https://doc.rust-lang.org/book/appendix-01-keywords.html). However, I can change any you feel need to be updated.

auto-merge was automatically disabled August 24, 2023 01:07

Head branch was pushed to by a user without write access

Copy link
Contributor

@eureka-cpu eureka-cpu left a comment

Choose a reason for hiding this comment

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

Hey @andrewvious really great work, checked all the links and they're correct.

I've requested just one change and I think this should be good to go.

docs/book/src/appendix/keywords.md Outdated Show resolved Hide resolved
eureka-cpu
eureka-cpu previously approved these changes Aug 25, 2023
Copy link
Contributor

@eureka-cpu eureka-cpu left a comment

Choose a reason for hiding this comment

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

LGTM nice job

Copy link
Contributor

@sarahschwartz sarahschwartz left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Can you update the links to the sway book so that they are all relative instead of absolute urls? This way the versions won't need to be updated.

@andrewvious
Copy link
Contributor Author

Thank you for your contribution! Can you update the links to the sway book so that they are all relative instead of absolute urls? This way the versions won't need to be updated.

I went ahead and set all the urls to the relative path in the book, but lost some precision the links had. Let me know if this works, or if there is a better way of doing it!

@Braqzen
Copy link
Contributor

Braqzen commented Aug 28, 2023

Thank you for your contribution! Can you update the links to the sway book so that they are all relative instead of absolute urls? This way the versions won't need to be updated.

I went ahead and set all the urls to the relative path in the book, but lost some precision the links had. Let me know if this works, or if there is a better way of doing it!

The reference seems to always point to a page instead of a specific heading. I don't recall if I did that because I couldn't link it precisely or because each section was meant to be a small page. If the page is too full then changing that page will inevitably lead to headings being changed and links being broken so it's better to structure content per page in a digestible manner.

I haven't tested the links but I wonder if we should be mixing the book and reference. The reference was meant to entirely replace the book, and hopefully it will before mainnet.

Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

Quickly glancing through CI the linter fails because the links start with /docs instead of ./docs (I assume, haven't checked)

@sarahschwartz
Copy link
Contributor

Thanks! I have one more nit: can you move this page into the existing Sway Reference section and remove the Appendix section?

@andrewvious
Copy link
Contributor Author

andrewvious commented Aug 29, 2023

Thanks! I have one more nit: can you move this page into the existing Sway Reference section and remove the Appendix section?

Would you mind further elaborating where inside the reference section you would prefer this to be moved to?
Should I create an Appendix in the Reference section?

@Braqzen
Copy link
Contributor

Braqzen commented Aug 29, 2023

Thanks! I have one more nit: can you move this page into the existing Sway Reference section and remove the Appendix section?

Would you mind further elaborating where inside the reference section you would prefer this to be moved to? Should I create an Appendix in the Reference section?

Move entire section from the book into the reference at the bottom

@sarahschwartz
Copy link
Contributor

Number 11 in the navigation of the Sway Book is "Sway Reference". You can move the keywords page into that section, add to the existing index.md with a link, and delete the apendix folder.
sway book

Copy link
Contributor

@eureka-cpu eureka-cpu left a comment

Choose a reason for hiding this comment

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

LGTM, nice work

@sarahschwartz sarahschwartz merged commit 098c244 into FuelLabs:master Aug 29, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request The Sway Book Everything to do with the Sway Book
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add appendix to the Sway book: Keywords
4 participants