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

docs(tut): Add cross ref to chapter 6 #550

Closed
wants to merge 1 commit into from

Conversation

VorpalBlade
Copy link
Contributor

@VorpalBlade VorpalBlade commented Jun 24, 2024

This helps when users try to use the docs as a reference to solve their issue at hand (as opposed to reading in order and remembering everything).

Related to discussion #544

This helps when users try to use the docs as a reference
to solve their issue at hand (as opposed to reading in
order and remembering everything).
@coveralls
Copy link

coveralls commented Jun 24, 2024

Pull Request Test Coverage Report for Build 9652374114

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.4%) to 42.044%

Files with Coverage Reduction New Missed Lines %
src/token/mod.rs 1 75.0%
src/macros/seq.rs 2 85.71%
Totals Coverage Status
Change from base Build 9407425121: 0.4%
Covered Lines: 1308
Relevant Lines: 3111

💛 - Coveralls

@VorpalBlade
Copy link
Contributor Author

I don't think that "coveralls" report is correct. I haven't changed any of the files it claims have new missed lines...

Comment on lines +570 to +572
//! As a reminder [`chapter_6`] demonstrated how to convert a [`PResult`] to a standard
//! Rust [`Result`], and this section assumes that has been done.
//!
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel this doesn't quite serve the intended role.

  • Everything up to this point was already relying on this in this chapter
  • If we had this, I feel like it should call out the mechanism whereby this is done or else it feels like its pointing back to nothing
  • I believe our callbacks like this are usually done when we reference much earlier chapters. What some chapters do for this type of thing is, at the very type, have a transition statement. For example, we could have something like "With Parser::parse converting PResult into Result for use in our application, let's see how we can polish the quality of the reported error message". Note that that example statement is meant only to serve as an example and I created it without much deep thought.

@VorpalBlade
Copy link
Contributor Author

Change apparently not wanted at all (seemed like an outright reject rather than suggestions for how to improve the PR) and I don't have time to work on this.

@epage
Copy link
Collaborator

epage commented Jul 24, 2024

To be clear, I gave a first-order approximation of how this could be reworked

For example, we could have something like
"With Parser::parse converting PResult into Result for use in our application, let's see how we can polish the quality of the reported error message".

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.

3 participants