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

Move toolchain architecture to markdown #4242

Merged
merged 16 commits into from
Sep 3, 2024

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Aug 22, 2024

Note I'm mostly trying to capture the docs as they exist today, not fixing issues with the docs. I think the doc itself hasn't changed much lately (i.e., for months). Trying to organize it a little better though, particularly so that it shows up reasonably when looking in github or the website.

}
```

The node order is (with indentation to indicate nesting):
Copy link

Choose a reason for hiding this comment

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

The following code block is missing the indentation from the original Google Doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Looks like this was getting auto-formatted away. Other examples are fine because they're not fully valid yaml.

- Start diagnostics with a capital letter or quoted code, and end them with a
period.

- Quoted code should be enclosed in backticks, for example: `"{0} is bad."`
Copy link

Choose a reason for hiding this comment

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

The backticks are missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

toolchain/docs/adding_features.md Outdated Show resolved Hide resolved
toolchain/docs/adding_features.md Outdated Show resolved Hide resolved
toolchain/docs/adding_features.md Outdated Show resolved Hide resolved
toolchain/docs/adding_features.md Outdated Show resolved Hide resolved
toolchain/docs/adding_features.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
@jonmeow jonmeow requested a review from zygoloid August 29, 2024 19:21
@jonmeow
Copy link
Contributor Author

jonmeow commented Aug 29, 2024

@zygoloid Maybe you can give your thoughts on the parse charts, at #4242 (comment)? Geoffrey is asking to go back to svg, I think the mermaid charts are good enough to understand what's going on, maybe you can weigh in.

@jonmeow
Copy link
Contributor Author

jonmeow commented Aug 30, 2024

@zygoloid Maybe you can give your thoughts on the parse charts, at #4242 (comment)? Geoffrey is asking to go back to svg, I think the mermaid charts are good enough to understand what's going on, maybe you can weigh in.

Just to note, we ended up discussing this in open discussion on Thursday. @chandlerc's suggested the ascii graphs.

toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
toolchain/docs/parse.md Outdated Show resolved Hide resolved
@eyelash
Copy link

eyelash commented Aug 30, 2024

Not sure what exactly was discussed in open discussion (can't find it in the minutes) but using box-drawing characters would probably make these ASCII diagrams easier to read.

For example:

        ┌───┐ ┌─────┐             ┌───┐ ┌───┐
        │ x │ │ i32 │             │ y │ │ 1 │
        └─┬─┘ └──┬──┘             └─┬─┘ └─┬─┘
          │      │                  │     │
          └──────┴──────┐           └─────┴─────┐
                        │                       │
┌─────┐               ┌─┴─┐ ┌───┐             ┌─┴─┐
│ var │               │ : │ │ = │             │ + │
└──┬──┘               └─┬─┘ └─┬─┘             └─┬─┘
   │                    │     │                 │
   └────────────────────┴─────┴─────────────────┴─────┐
                                                      │
                                                    ┌─┴─┐
                                                    │ ; │
                                                    └───┘

@jonmeow
Copy link
Contributor Author

jonmeow commented Sep 3, 2024

Not sure what exactly was discussed in open discussion (can't find it in the minutes) but using box-drawing characters would probably make these ASCII diagrams easier to read.

For example:

    ┌───┐ ┌─────┐             ┌───┐ ┌───┐
    │ x │ │ i32 │             │ y │ │ 1 │
    └─┬─┘ └──┬──┘             └─┬─┘ └─┬─┘
      │      │                  │     │
      └──────┴──────┐           └─────┴─────┐
                    │                       │

┌─────┐ ┌─┴─┐ ┌───┐ ┌─┴─┐
│ var │ │ : │ │ = │ │ + │
└──┬──┘ └─┬─┘ └─┬─┘ └─┬─┘
│ │ │ │
└────────────────────┴─────┴─────────────────┴─────┐

┌─┴─┐
│ ; │
└───┘

This kind of character set change would require a chunk of time to just rewrite the graphs, and would be harder to update versus the basic ascii currently in use. It might make sense to consider separately, but given this review is jammed on specific style of these graphs, I want to be careful about investing lots of time into aesthetics that might be undone.

@geoffromer geoffromer added this pull request to the merge queue Sep 3, 2024
Merged via the queue into carbon-language:trunk with commit a24816a Sep 3, 2024
8 checks passed
@jonmeow jonmeow deleted the toolchain-docs branch September 3, 2024 23:20
dwblaikie pushed a commit to dwblaikie/carbon-lang that referenced this pull request Sep 6, 2024
Note I'm mostly trying to capture [the
docs](https://docs.google.com/document/d/1RRYMm42osyqhI2LyjrjockYCutQ5dOf8Abu50kTrkX0/edit?resourcekey=0-kHyqOESbOHmzZphUbtLrTw&tab=t.0)
as they exist today, not fixing issues with the docs. I think the doc
itself hasn't changed much lately (i.e., for months). Trying to organize
it a little better though, particularly so that it shows up reasonably
when looking in github or the website.

---------

Co-authored-by: Geoff Romer <[email protected]>
dwblaikie pushed a commit to dwblaikie/carbon-lang that referenced this pull request Sep 9, 2024
Note I'm mostly trying to capture [the
docs](https://docs.google.com/document/d/1RRYMm42osyqhI2LyjrjockYCutQ5dOf8Abu50kTrkX0/edit?resourcekey=0-kHyqOESbOHmzZphUbtLrTw&tab=t.0)
as they exist today, not fixing issues with the docs. I think the doc
itself hasn't changed much lately (i.e., for months). Trying to organize
it a little better though, particularly so that it shows up reasonably
when looking in github or the website.

---------

Co-authored-by: Geoff Romer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants