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

Proofreading fixes #109

Merged
merged 19 commits into from
Aug 1, 2023
Merged

Conversation

SergioGasquez
Copy link
Member

@SergioGasquez SergioGasquez commented Jul 25, 2023

Thanks @f-hollow for all the suggestions.

Regarding IDEs:

I read this paragraph and immediately have a question why only VS Code is covered?

Does this tool have a better support? Is it a personal preference of the authors?

We only cover VsCode as it's the most used IDE. In the past we had subsections for other IDEs, but they were very short and didn't provide much, so we decided to get rid of them.

Regarding debugging chapter comments:

As I see the table, I immediately have a question: If openOCD and VS Code support all chips, then why bother with probe-rs?

Are there any advantages to it? Should they be mentioned in the introduction or are they obvious to the target audience?

How openOCD and VS Code are different and why we need both?

I added an entry for this in #105

I see that the subsections mention certain chips. Does it mean that other chips do not require any actions on the part of the user?

Should it be mentioned anywhere?

I added an entry for this in #105

Copy link
Member Author

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

Here I address some of your comments/suggestions

src/introduction.md Show resolved Hide resolved
src/overview/index.md Show resolved Hide resolved
src/overview/using-the-standard-library.md Show resolved Hide resolved
@f-hollow
Copy link
Contributor

@SergioGasquez

We only cover VsCode as it's the most used IDE. In the past we had subsections for other IDEs, but they were very short and didn't provide much, so we decided to get rid of them.

Suggest briefly mentioning this:

We chose VS Code because it has better support for Rust and is more popular. However, you can use other IDEs....

@f-hollow
Copy link
Contributor

There is also one comment about the Tips and Tricks section in /src/tooling/text-editors-and-ides.md

Looks like this section contains two tips:

  • Issue with std and Rust Analyzer
  • Custom toolchain and Cargo hints

Suggest splitting "Tips and Tricks" into two sections. As these sections relate to VS Code, suggest turning them into subsections VS Code.

@f-hollow
Copy link
Contributor

f-hollow commented Jul 26, 2023

The only comment that we did not cover is about the word ecosystem in "Ecosystem Overview."

The Ecosystem Overview chapter seems to cover the std and no_std development approaches. Can it be called an ecosystem? How about "Overview of Development Approaches" or something else?

Copy link
Contributor

@f-hollow f-hollow 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 implementing my suggestions. Now only a few small things left.

src/overview/using-the-standard-library.md Outdated Show resolved Hide resolved
src/tooling/debugging/vscode-debugging.md Outdated Show resolved Hide resolved
@SergioGasquez
Copy link
Member Author

Suggest briefly mentioning this:

We chose VS Code because it has better support for Rust and is more popular. However, you can use other IDEs....

Suggest splitting "Tips and Tricks" into two sections. As these sections relate to VS Code, suggest turning them into subsections VS Code.

Both things should be fixed in 2cf23e4

@SergioGasquez
Copy link
Member Author

The Ecosystem Overview chapter seems to cover the std and no_std development approaches. Can it be called an ecosystem? How about "Overview of Development Approaches" or something else?

Updated!

@SergioGasquez SergioGasquez linked an issue Jul 26, 2023 that may be closed by this pull request
@f-hollow
Copy link
Contributor

@SergioGasquez

The following steps might be (in this or separate PRs):

  1. (Optional) Ask others to review the current updates.
  2. Lint the content using Vale linter.
    • The configuration files are in idf/esp-vale-config
    • As I haven't written the instructions yet, suggest having a call to discuss how to use Vale
  3. Update file names and folder names in src/
    • Some names are outdated, so it is not easy to navigate inside src/
    • Consider also adding chapter numbers in file/folder names to arrange items in the order the book uses them

@SergioGasquez
Copy link
Member Author

Just pushed 1b9e9be updating file names and folder names in src/and adding some redirections from old paths to new paths, so we maintain backward compatibility and do not break links pointing to the book. We already did break some links in the past, see esp-rs/esp-idf-template#137 for example.

With these redirections #72 can be also closed when this is merged.

Adding numbers to the folder/file names will require adding a lot of redirections, and if we include/remove a chapter in the future, it will include a lot more redirections, for this reason I would prefer not to include them.

@SergioGasquez SergioGasquez linked an issue Jul 27, 2023 that may be closed by this pull request
@f-hollow
Copy link
Contributor

f-hollow commented Jul 28, 2023

@SergioGasquez

Did another review and came up with a few more nitpicks. This is the last batch from me.

I uploaded a file with comments in Code Review format on mattermost.

The book looks great!

P.S. Just discovered that I can also attach files here. It is even more convenient.

code-review.csv
code-review.csv.md

@SergioGasquez
Copy link
Member Author

@SergioGasquez

Did another review and came up with a few more nitpicks. This is the last batch from me.

I uploaded a file with comments in Code Review format on mattermost.

The book looks great!

P.S. Just discovered that I can also attach files here. It is even more convenient.

code-review.csv code-review.csv.md

Just addressed all your comments/suggestions

@SergioGasquez SergioGasquez force-pushed the feature/proofreading branch 2 times, most recently from 8395df7 to 67f4e97 Compare July 31, 2023 09:17
@f-hollow
Copy link
Contributor

@SergioGasquez

Thank you for implementing all the suggestions! One thing: You fixed the issue with the unnecessary scroll bar in the code snippet in Understanding esp-template. Please do the same in Understanding esp-idf-template.

Now that you have formatted the notes and warnings according to the Rust Documentation Style Guide, the book looks as official as all other Rust books.

@SergioGasquez
Copy link
Member Author

Thank you for implementing all the suggestions! One thing: You fixed the issue with the unnecessary scroll bar in the code snippet in Understanding esp-template. Please do the same in Understanding esp-idf-template.

Good catch! Fixed.

@SergioGasquez SergioGasquez merged commit 838ed9a into esp-rs:main Aug 1, 2023
@SergioGasquez SergioGasquez deleted the feature/proofreading branch August 1, 2023 07:52
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.

Apply guidelines, fix formatting and update chapter titles Try to redirect to updated pages where possible
2 participants